cron job: media_tree daily build: ERRORS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Wed Oct 12 05:00:22 CEST 2016 media-tree git hash:9fce0c226536fc36c7fb0a8ca38a995be43e media_build git hash: ecfc9bfca3012b0c6e19967ce90f621f71a6da94 v4l-utils git hash: 7c2664b9a9b411d8b183009146e4f8548ca1d81a gcc version:i686-linux-gcc (GCC) 6.2.0 sparse version: v0.5.0-3553-g78b2ea6 smatch version: v0.5.0-3553-g78b2ea6 host hardware: x86_64 host os:4.7.0-164 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: WARNINGS linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.36.4-i686: WARNINGS linux-2.6.37.6-i686: WARNINGS linux-2.6.38.8-i686: ERRORS linux-2.6.39.4-i686: WARNINGS linux-3.0.60-i686: WARNINGS linux-3.1.10-i686: ERRORS linux-3.2.37-i686: ERRORS linux-3.3.8-i686: ERRORS linux-3.4.27-i686: WARNINGS linux-3.5.7-i686: WARNINGS linux-3.6.11-i686: WARNINGS linux-3.7.4-i686: WARNINGS linux-3.8-i686: WARNINGS linux-3.9.2-i686: WARNINGS linux-3.10.1-i686: WARNINGS linux-3.11.1-i686: OK linux-3.13.11-i686: WARNINGS linux-3.14.9-i686: WARNINGS linux-3.15.2-i686: WARNINGS linux-3.16.7-i686: WARNINGS linux-3.17.8-i686: WARNINGS linux-3.18.7-i686: WARNINGS linux-3.19-i686: WARNINGS linux-4.0.9-i686: WARNINGS linux-4.1.33-i686: WARNINGS linux-4.2.8-i686: WARNINGS linux-4.3.6-i686: WARNINGS linux-4.4.22-i686: WARNINGS linux-4.5.7-i686: WARNINGS linux-4.6.7-i686: WARNINGS linux-4.7.5-i686: WARNINGS linux-4.8-i686: OK linux-2.6.36.4-x86_64: WARNINGS linux-2.6.37.6-x86_64: WARNINGS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-x86_64: WARNINGS linux-3.0.60-x86_64: WARNINGS linux-3.1.10-x86_64: ERRORS linux-3.2.37-x86_64: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.27-x86_64: WARNINGS linux-3.5.7-x86_64: WARNINGS linux-3.6.11-x86_64: WARNINGS linux-3.7.4-x86_64: WARNINGS linux-3.8-x86_64: WARNINGS linux-3.9.2-x86_64: WARNINGS linux-3.10.1-x86_64: WARNINGS linux-3.11.1-x86_64: OK linux-3.13.11-x86_64: WARNINGS linux-3.14.9-x86_64: WARNINGS linux-3.15.2-x86_64: WARNINGS linux-3.16.7-x86_64: WARNINGS linux-3.17.8-x86_64: WARNINGS linux-3.18.7-x86_64: WARNINGS linux-3.19-x86_64: WARNINGS linux-4.0.9-x86_64: WARNINGS linux-4.1.33-x86_64: WARNINGS linux-4.2.8-x86_64: WARNINGS linux-4.3.6-x86_64: WARNINGS linux-4.4.22-x86_64: WARNINGS linux-4.5.7-x86_64: WARNINGS linux-4.6.7-x86_64: WARNINGS linux-4.7.5-x86_64: WARNINGS linux-4.8-x86_64: OK apps: WARNINGS spec-git: OK smatch: ERRORS 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/index.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: [RFC 6/6] drivers: staging: ion: add ION_IOC_TAG ioctl
On Wednesday, October 12, 2016 7:50 AM Ruchi Kandoi wrote: > +/** > + * struct ion_fd_data - metadata passed from userspace for a handle s/fd/tag/ ? > + * @handle: a handle > + * @tag: a string describing the buffer > + * > + * For ION_IOC_TAG userspace populates the handle field with > + * the handle returned from ion alloc and type contains the memtrack_type > which > + * accurately describes the usage for the memory. > + */ > +struct ion_tag_data { > + ion_user_handle_t handle; > + char tag[ION_MAX_TAG_LEN]; > +}; > + -- 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 v2.1 03/31] cinergyT2-core: handle error code on RC query
There's no sense on decoding and generating a RC key code if there was an error on the URB control message. Signed-off-by: Mauro Carvalho Chehabdiff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c index d85c0c4d4042..8ac825413d5a 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c @@ -102,7 +102,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap) /* Copy this pointer as we are gonna need it in the release phase */ cinergyt2_usb_device = adap->dev; - return 0; + return ret; } static struct rc_map_table rc_map_cinergyt2_table[] = { @@ -162,14 +162,16 @@ static int repeatable_keys[] = { static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { struct cinergyt2_state *st = d->priv; - int i; + int i, ret; *state = REMOTE_NO_KEY_PRESSED; mutex_lock(>data_mutex); st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; - dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + if (ret < 0) + goto ret; if (st->data[4] == 0xff) { /* key repeat */ -- 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 v2 28/31] cpia2_usb: don't use stack for DMA
Em Tue, 11 Oct 2016 22:56:06 + Kosuke Tatsukawaescreveu: > Hi, > > > The USB control messages require DMA to work. We cannot pass > > a stack-allocated buffer, as it is not warranted that the > > stack would be into a DMA enabled area. > > > > Signed-off-by: Mauro Carvalho Chehab > > Signed-off-by: Mauro Carvalho Chehab > > --- > > drivers/media/usb/cpia2/cpia2_usb.c | 32 +--- > > 1 file changed, 29 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/cpia2/cpia2_usb.c > > b/drivers/media/usb/cpia2/cpia2_usb.c > > index 13620cdf0599..417d683b237d 100644 > > --- a/drivers/media/usb/cpia2/cpia2_usb.c > > +++ b/drivers/media/usb/cpia2/cpia2_usb.c > > @@ -545,10 +545,19 @@ static void free_sbufs(struct camera_data *cam) > > static int write_packet(struct usb_device *udev, > > u8 request, u8 * registers, u16 start, size_t size) > > { > > + unsigned char *buf; > > + int ret; > > + > > if (!registers || size <= 0) > > return -EINVAL; > > > > - return usb_control_msg(udev, > > + buf = kmalloc(size, GFP_KERNEL); > > + if (!buf) > > + return -ENOMEM; > > + > > + memcpy(buf, registers, size); > > + > > + ret = usb_control_msg(udev, > >usb_sndctrlpipe(udev, 0), > >request, > >USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > @@ -557,6 +566,9 @@ static int write_packet(struct usb_device *udev, > >registers, /* buffer */ >= > > I think you also want to change the argument to usb_control_msg() from > "registers" to "buf" in write_packet(). True! Thanks for pointing it! Just sent a version of the patch with this fixed. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 .1 28/31] cpia2_usb: don't use stack for DMA
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab-- v2.1: As pointed by Kosuke Tatsukawa, I forgot to replace "registers" var to "buf" at one of the usb_control_msg() calls. diff --git a/drivers/media/usb/cpia2/cpia2_usb.c b/drivers/media/usb/cpia2/cpia2_usb.c index 13620cdf0599..e9100a235831 100644 --- a/drivers/media/usb/cpia2/cpia2_usb.c +++ b/drivers/media/usb/cpia2/cpia2_usb.c @@ -545,18 +545,30 @@ static void free_sbufs(struct camera_data *cam) static int write_packet(struct usb_device *udev, u8 request, u8 * registers, u16 start, size_t size) { + unsigned char *buf; + int ret; + if (!registers || size <= 0) return -EINVAL; - return usb_control_msg(udev, + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + memcpy(buf, registers, size); + + ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), request, USB_TYPE_VENDOR | USB_RECIP_DEVICE, start, /* value */ 0, /* index */ - registers, /* buffer */ + buf, /* buffer */ size, HZ); + + kfree(buf); + return ret; } / @@ -567,18 +579,32 @@ static int write_packet(struct usb_device *udev, static int read_packet(struct usb_device *udev, u8 request, u8 * registers, u16 start, size_t size) { + unsigned char *buf; + int ret; + if (!registers || size <= 0) return -EINVAL; - return usb_control_msg(udev, + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), request, USB_DIR_IN|USB_TYPE_VENDOR|USB_RECIP_DEVICE, start, /* value */ 0, /* index */ - registers, /* buffer */ + buf, /* buffer */ size, HZ); + + if (ret >= 0) + memcpy(registers, buf, size); + + kfree(buf); + + return ret; } /** -- 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: [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169
Em Tue, 11 Oct 2016 23:41:53 +0200 (CEST) Julia Lawallescreveu: > On Tue, 11 Oct 2016, Mauro Carvalho Chehab wrote: > > > Em Tue, 11 Oct 2016 15:16:24 +0200 (CEST) > > Julia Lawall escreveu: > > > > > On Tue, 11 Oct 2016, Julia Lawall wrote: > > > > > > > It looks like a lock may be needed before line 174. > > > > > > Sorry, an unlock. > > > > I suspect that this is a false positive warning, as there is a > > mutex unlock on the same routine, at line 203. All exit > > conditions go to the unlock condition. > > There is a direct exit in line 174. Ah! I was looking at the wrong patch (cinergyT2-core: don't do DMA on stack). Thanks for pointing it. I'll the affected code to: - dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + if (ret < 0) + goto ret; Regards, Mauro -- Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/6] Module for tracking/accounting shared memory buffers
On Tue, Oct 11, 2016 at 7:50 PM, Ruchi Kandoiwrote: > This patchstack introduces a new "memtrack" module for tracking and accounting > memory exported to userspace as shared buffers, like dma-buf fds or GEM > handles. btw, I wouldn't care much about the non-dmabuf case.. dri2/flink is kind of legacy and the sharing patterns there are not so complex that we have found the need for any more elaborate debug infrastructure than what we already have. Between existing dmabuf debugfs, and /proc/*/maps (and /proc/*/fd?), I wonder what is missing? Maybe there is a less intrusive way to get at the debugging info you want? BR, -R > Any process holding a reference to these buffers will keep the kernel from > reclaiming its backing pages. mm counters don't provide a complete picture of > these allocations, since they only account for pages that are mapped into a > process's address space. This problem is especially bad for systems like > Android that use dma-buf fds to share graphics and multimedia buffers between > processes: these allocations are often large, have complex sharing patterns, > and are rarely mapped into every process that holds a reference to them. > > memtrack maintains a per-process list of shared buffer references, which is > exported to userspace as /proc/[pid]/memtrack. Buffers can be optionally > "tagged" with a short string: for example, Android userspace would use this > tag to identify whether buffers were allocated on behalf of the camera stack, > GL, etc. memtrack also exports the VMAs associated with these buffers so > that pages already included in the process's mm counters aren't > double-counted. > > Shared-buffer allocators can hook into memtrack by embedding > struct memtrack_buffer in their buffer metadata, calling > memtrack_buffer_{init,remove} at buffer allocation and free time, and > memtrack_buffer_{install,uninstall} when a userspace process takes or > drops a reference to the buffer. For fd-backed buffers like dma-bufs, hooks > in > fdtable.c and fork.c automatically notify memtrack when references are added > or > removed from a process's fd table. > > This patchstack adds memtrack hooks into dma-buf and ion. If there's upstream > interest in memtrack, it can be extended to other memory allocators as well, > such as GEM implementations. > > Greg Hackmann (1): > drivers: staging: ion: add ION_IOC_TAG ioctl > > Ruchi Kandoi (5): > fs: add installed and uninstalled file_operations > drivers: misc: add memtrack > dma-buf: add memtrack support > memtrack: Adds the accounting to keep track of all mmaped/unmapped > pages. > memtrack: Add memtrack accounting for forked processes. > > drivers/android/binder.c| 4 +- > drivers/dma-buf/dma-buf.c | 37 +++ > drivers/misc/Kconfig| 16 + > drivers/misc/Makefile | 1 + > drivers/misc/memtrack.c | 516 > > drivers/staging/android/ion/ion-ioctl.c | 17 ++ > drivers/staging/android/ion/ion.c | 60 +++- > drivers/staging/android/ion/ion_priv.h | 2 + > drivers/staging/android/uapi/ion.h | 25 ++ > fs/file.c | 38 ++- > fs/open.c | 2 +- > fs/proc/base.c | 4 + > include/linux/dma-buf.h | 5 + > include/linux/fdtable.h | 4 +- > include/linux/fs.h | 2 + > include/linux/memtrack.h| 130 > include/linux/mm.h | 3 + > include/linux/sched.h | 3 + > kernel/fork.c | 23 +- > 19 files changed, 875 insertions(+), 17 deletions(-) > create mode 100644 drivers/misc/memtrack.c > create mode 100644 include/linux/memtrack.h > > -- > 2.8.0.rc3.226.g39d4020 > -- 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: [RFC 0/6] Module for tracking/accounting shared memory buffers
On Tue, Oct 11, 2016 at 04:50:04PM -0700, Ruchi Kandoi wrote: > memtrack maintains a per-process list of shared buffer references, which is > exported to userspace as /proc/[pid]/memtrack. Buffers can be optionally > "tagged" with a short string: for example, Android userspace would use this > tag to identify whether buffers were allocated on behalf of the camera stack, > GL, etc. memtrack also exports the VMAs associated with these buffers so > that pages already included in the process's mm counters aren't > double-counted. > > Shared-buffer allocators can hook into memtrack by embedding > struct memtrack_buffer in their buffer metadata, calling > memtrack_buffer_{init,remove} at buffer allocation and free time, and > memtrack_buffer_{install,uninstall} when a userspace process takes or > drops a reference to the buffer. For fd-backed buffers like dma-bufs, hooks > in > fdtable.c and fork.c automatically notify memtrack when references are added > or > removed from a process's fd table. > > This patchstack adds memtrack hooks into dma-buf and ion. If there's upstream > interest in memtrack, it can be extended to other memory allocators as well, > such as GEM implementations. No, with a side of Hell, No. Not to mention anything else, * descriptor tables do not belong to any specific task_struct and actions done by one show up in all who share that thing. * shared descriptor table does not imply belonging to the same group. * shared descriptor table can become unshared at any point, invisibly for that Fine Piece Of Software. * while we are at it, blocking allocation under several spinlocks (and with interrupts disabled, for good measure) is generally considered a bloody bad idea. That - just from the quick look through that patchset. Bringing task_struct into the API is already sufficient for a NAK. -- 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
[RFC 2/6] drivers: misc: add memtrack
Shared-buffer allocators like ion or GEM traditionally call into CMA or alloc_pages() to get backing memory, meaning these allocations will not show up in any process's mm counters. But since these allocations are often used for things like graphics buffers that can be extremely large, the user just sees a bunch of pages vanishing from the system without an explanation. CONFIG_MEMTRACK adds infrastructure for "blaming" these allocations back to the processes currently holding a reference to the shared buffer. This information is exposed to userspace through /proc/[pid]/memtrack. To use memtrack, the shared memory allocator should: (1) Embed a struct memtrack_buffer somewhere in the underlying buffer's metadata, and initialize it with memtrack_buffer_init() (3) Call memtrack_buffer_{install,uninstall} each time a task takes or drops a reference to the shared buffer (3) Call memtrack_buffer_remove() before destroying a tracked buffer CONFIG_MEMTRACK_DEBUG adds a global list of all buffers tracked by memtrack, accessible through /sys/kernel/debug/memtrack. This involves maintaining a global idr of buffers. Due to the extra overhead, CONFIG_MEMTRACK_DEBUG is intended for debugging memory leaks rather than production use. Signed-off-by: Greg HackmannSigned-off-by: Ruchi Kandoi --- drivers/misc/Kconfig | 16 +++ drivers/misc/Makefile| 1 + drivers/misc/memtrack.c | 360 +++ fs/proc/base.c | 4 + include/linux/memtrack.h | 94 + include/linux/sched.h| 3 + kernel/fork.c| 4 + 7 files changed, 482 insertions(+) create mode 100644 drivers/misc/memtrack.c create mode 100644 include/linux/memtrack.h diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 64971ba..7557fb1 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -766,6 +766,22 @@ config PANEL_BOOT_MESSAGE An empty message will only clear the display at driver init time. Any other printf()-formatted message is valid with newline and escape codes. +config MEMTRACK + tristate "Per-pid memory statistics" + default n + ---help--- + Keeps track of shared buffers allocated by the process and + exports them via /proc//memtrack. + +config MEMTRACK_DEBUG + tristate "Per-pid memory statistics debug option" + depends on MEMTRACK && DEBUG_FS + default n + ---help--- + Keeps track of all shared buffers allocated and exports the list + via /sys/kernel/debug/memtrack. + + source "drivers/misc/c2port/Kconfig" source "drivers/misc/c2port/Kconfig" source "drivers/misc/eeprom/Kconfig" source "drivers/misc/cb710/Kconfig" diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 3198336..1fbb084 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -68,3 +68,4 @@ OBJCOPYFLAGS_lkdtm_rodata_objcopy.o := \ targets += lkdtm_rodata.o lkdtm_rodata_objcopy.o $(obj)/lkdtm_rodata_objcopy.o: $(obj)/lkdtm_rodata.o FORCE $(call if_changed,objcopy) +obj-$(CONFIG_MEMTRACK) += memtrack.o diff --git a/drivers/misc/memtrack.c b/drivers/misc/memtrack.c new file mode 100644 index 000..e5c7e03 --- /dev/null +++ b/drivers/misc/memtrack.c @@ -0,0 +1,360 @@ +/* drivers/misc/memtrack.c + * + * Copyright (C) 2016 Google, Inc. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * 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. + * + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct memtrack_handle { + struct memtrack_buffer *buffer; + struct rb_node node; + struct rb_root *root; + struct kref refcount; +}; + +static struct kmem_cache *memtrack_handle_cache; + +static DEFINE_MUTEX(memtrack_id_lock); +#if IS_ENABLED(CONFIG_MEMTRACK_DEBUG) +static struct dentry *debugfs_file; +static DEFINE_IDR(mem_idr); +#else +static DEFINE_IDA(mem_ida); +#endif + +static void memtrack_buffer_install_locked(struct rb_root *root, + struct memtrack_buffer *buffer) +{ + struct rb_node **new = >rb_node, *parent = NULL; + struct memtrack_handle *handle; + + while (*new) { + struct rb_node *node = *new; + + handle = rb_entry(node, struct memtrack_handle, node); + parent = node; + if (handle->buffer->id > buffer->id) { + new = >rb_left; + } else if (handle->buffer->id < buffer->id) { + new = >rb_right; +
[RFC 3/6] dma-buf: add memtrack support
Signed-off-by: Greg HackmannSigned-off-by: Ruchi Kandoi --- drivers/dma-buf/dma-buf.c | 37 ++ drivers/staging/android/ion/ion.c | 14 + drivers/staging/android/ion/ion_priv.h | 2 ++ include/linux/dma-buf.h| 5 + 4 files changed, 58 insertions(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index ddaee60..f632c2b 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -297,12 +297,32 @@ static long dma_buf_ioctl(struct file *file, } } +static void dma_buf_installed(struct file *file, struct task_struct *task) +{ + struct memtrack_buffer *memtrack = + dma_buf_memtrack_buffer(file->private_data); + + if (memtrack) + memtrack_buffer_install(memtrack, task); +} + +static void dma_buf_uninstalled(struct file *file, struct task_struct *task) +{ + struct memtrack_buffer *memtrack = + dma_buf_memtrack_buffer(file->private_data); + + if (memtrack) + memtrack_buffer_uninstall(memtrack, task); +} + 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, .unlocked_ioctl = dma_buf_ioctl, + .installed = dma_buf_installed, + .uninstalled= dma_buf_uninstalled, }; /* @@ -830,6 +850,23 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr) } EXPORT_SYMBOL_GPL(dma_buf_vunmap); +/** + * dma_buf_memtrack_buffer - returns a memtrack entry associated with dma_buf + * + * @dmabuf:[in]pointer to dma_buf + * + * Returns the struct memtrack_buffer associated with this dma_buf's + * backing pages. If memtrack isn't enabled in the kernel, or the dma_buf + * exporter doesn't have memtrack support, returns NULL. + */ +struct memtrack_buffer *dma_buf_memtrack_buffer(struct dma_buf *dmabuf) +{ + if (!dmabuf->ops->memtrack_buffer) + return NULL; + return dmabuf->ops->memtrack_buffer(dmabuf); +} +EXPORT_SYMBOL_GPL(dma_buf_memtrack_buffer); + #ifdef CONFIG_DEBUG_FS static int dma_buf_debug_show(struct seq_file *s, void *unused) { diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 396ded5..1c2df54 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -196,6 +196,7 @@ void ion_buffer_destroy(struct ion_buffer *buffer) buffer->heap->ops->unmap_kernel(buffer->heap, buffer); buffer->heap->ops->free(buffer); vfree(buffer->pages); + memtrack_buffer_remove(>memtrack_buffer); kfree(buffer); } @@ -458,6 +459,8 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len, handle = ERR_PTR(ret); } + memtrack_buffer_init(>memtrack_buffer, len); + return handle; } EXPORT_SYMBOL(ion_alloc); @@ -1013,6 +1016,16 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf, return 0; } +static struct memtrack_buffer *ion_memtrack_buffer(struct dma_buf *buffer) +{ + if (IS_ENABLED(CONFIG_MEMTRACK) && buffer && buffer->priv) { + struct ion_buffer *ion_buffer = buffer->priv; + + return _buffer->memtrack_buffer; + } + return NULL; +} + static struct dma_buf_ops dma_buf_ops = { .map_dma_buf = ion_map_dma_buf, .unmap_dma_buf = ion_unmap_dma_buf, @@ -1024,6 +1037,7 @@ static struct dma_buf_ops dma_buf_ops = { .kunmap_atomic = ion_dma_buf_kunmap, .kmap = ion_dma_buf_kmap, .kunmap = ion_dma_buf_kunmap, + .memtrack_buffer = ion_memtrack_buffer, }; struct dma_buf *ion_share_dma_buf(struct ion_client *client, diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h index 3c3b324..74c38eb 100644 --- a/drivers/staging/android/ion/ion_priv.h +++ b/drivers/staging/android/ion/ion_priv.h @@ -27,6 +27,7 @@ #include #include #include +#include #include "ion.h" @@ -78,6 +79,7 @@ struct ion_buffer { int handle_count; char task_comm[TASK_COMM_LEN]; pid_t pid; + struct memtrack_buffer memtrack_buffer; }; void ion_buffer_destroy(struct ion_buffer *buffer); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index e0b0741..dfcc2d0 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -32,6 +32,7 @@ #include #include #include +#include struct device; struct dma_buf; @@ -70,6 +71,8 @@ struct dma_buf_attachment; * @vmap: [optional] creates a virtual mapping for the buffer into kernel * address space. Same restrictions as for vmap and friends apply. * @vunmap: [optional] unmaps a vmap from the buffer + * @memtrack_buffer: [optional] returns the
[RFC 5/6] memtrack: Add memtrack accounting for forked processes.
When a process is forked, all the buffers are shared with the forked process too. Adds the functionality to add memtrack accounting for the forked processes. Forked process gets a copy of the mapped pages of the parent process. This patch makes sure that the new mapped pages are attributed to the child process instead of the parent. Signed-off-by: Ruchi Kandoi--- drivers/misc/memtrack.c | 45 +++ drivers/staging/android/ion/ion.c | 45 +-- include/linux/memtrack.h | 19 +++-- include/linux/mm.h| 3 +++ kernel/fork.c | 19 +++-- 5 files changed, 117 insertions(+), 14 deletions(-) diff --git a/drivers/misc/memtrack.c b/drivers/misc/memtrack.c index 4b2d17f..fa2601a 100644 --- a/drivers/misc/memtrack.c +++ b/drivers/misc/memtrack.c @@ -204,12 +204,13 @@ EXPORT_SYMBOL(memtrack_buffer_uninstall); * @buffer: the buffer's memtrack entry * * @vma: vma being opened + * @task: task which mapped the pages */ void memtrack_buffer_vm_open(struct memtrack_buffer *buffer, - const struct vm_area_struct *vma) + const struct vm_area_struct *vma, struct task_struct *task) { unsigned long flags; - struct task_struct *leader = current->group_leader; + struct task_struct *leader = task->group_leader; struct memtrack_vma_list *vma_list; vma_list = kmalloc(sizeof(*vma_list), GFP_KERNEL); @@ -228,12 +229,13 @@ EXPORT_SYMBOL(memtrack_buffer_vm_open); * * @buffer: the buffer's memtrack entry * @vma: the vma being closed + * @task: task that mmaped the pages */ void memtrack_buffer_vm_close(struct memtrack_buffer *buffer, - const struct vm_area_struct *vma) + const struct vm_area_struct *vma, struct task_struct *task) { unsigned long flags; - struct task_struct *leader = current->group_leader; + struct task_struct *leader = task->group_leader; write_lock_irqsave(>memtrack_lock, flags); memtrack_buffer_vm_close_locked(>memtrack_rb, buffer, vma); @@ -241,6 +243,41 @@ void memtrack_buffer_vm_close(struct memtrack_buffer *buffer, } EXPORT_SYMBOL(memtrack_buffer_vm_close); +/** + * memtrack_buffer_install_fork - Install all parent's handles into + * child. + * + * @parent: parent task + * @child: child task + */ +void memtrack_buffer_install_fork(struct task_struct *parent, + struct task_struct *child) +{ + struct task_struct *leader, *leader_child; + struct rb_root *root; + struct rb_node *node; + unsigned long flags; + + if (!child || !parent) + return; + + leader = parent->group_leader; + leader_child = child->group_leader; + write_lock_irqsave(>memtrack_lock, flags); + root = >memtrack_rb; + node = rb_first(root); + while (node) { + struct memtrack_handle *handle; + + handle = rb_entry(node, struct memtrack_handle, node); + memtrack_buffer_install_locked(_child->memtrack_rb, + handle->buffer); + node = rb_next(node); + } + write_unlock_irqrestore(>memtrack_lock, flags); +} +EXPORT_SYMBOL(memtrack_buffer_install_fork); + static int memtrack_id_alloc(struct memtrack_buffer *buffer) { int ret; diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index c32d520..451aa0f 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -906,7 +906,7 @@ static void ion_vm_open(struct vm_area_struct *vma) list_add(_list->list, >vmas); mutex_unlock(>lock); pr_debug("%s: adding %p\n", __func__, vma); - memtrack_buffer_vm_open(>memtrack_buffer, vma); + memtrack_buffer_vm_open(>memtrack_buffer, vma, current); } static void ion_vm_close(struct vm_area_struct *vma) @@ -925,13 +925,51 @@ static void ion_vm_close(struct vm_area_struct *vma) break; } mutex_unlock(>lock); - memtrack_buffer_vm_close(>memtrack_buffer, vma); + memtrack_buffer_vm_close(>memtrack_buffer, vma, current); +} + +void vm_track(struct vm_area_struct *vma, struct task_struct *task) +{ + struct ion_buffer *buffer = vma->vm_private_data; + + memtrack_buffer_vm_open(>memtrack_buffer, vma, task); +} + +void vm_untrack(struct vm_area_struct *vma, struct task_struct *task) +{ + struct ion_buffer *buffer = vma->vm_private_data; + + memtrack_buffer_vm_close(>memtrack_buffer, vma, task); } static const struct vm_operations_struct ion_vma_ops = { .open = ion_vm_open, .close = ion_vm_close, .fault = ion_vm_fault, + .track = vm_track, + .untrack = vm_untrack, +}; + +static void memtrack_vm_close(struct vm_area_struct *vma) +{ + struct ion_buffer *buffer =
[RFC 4/6] memtrack: Adds the accounting to keep track of all mmaped/unmapped pages.
Since mmaped pages will be accounted by the PSS, memtrack needs a way to differentiate the total memory that hasn't been accounted for. Signed-off-by: Ruchi KandoiSigned-off-by: Greg Hackmann --- drivers/misc/memtrack.c | 175 -- drivers/staging/android/ion/ion.c | 5 +- include/linux/memtrack.h | 29 +++ 3 files changed, 180 insertions(+), 29 deletions(-) diff --git a/drivers/misc/memtrack.c b/drivers/misc/memtrack.c index e5c7e03..4b2d17f 100644 --- a/drivers/misc/memtrack.c +++ b/drivers/misc/memtrack.c @@ -22,12 +22,19 @@ #include #include #include +#include + +struct memtrack_vma_list { + struct hlist_node node; + const struct vm_area_struct *vma; +}; struct memtrack_handle { struct memtrack_buffer *buffer; struct rb_node node; struct rb_root *root; struct kref refcount; + struct hlist_head vma_list; }; static struct kmem_cache *memtrack_handle_cache; @@ -40,8 +47,8 @@ static DEFINE_IDR(mem_idr); static DEFINE_IDA(mem_ida); #endif -static void memtrack_buffer_install_locked(struct rb_root *root, - struct memtrack_buffer *buffer) +static struct memtrack_handle *memtrack_handle_find_locked(struct rb_root *root, + struct memtrack_buffer *buffer, bool alloc) { struct rb_node **new = >rb_node, *parent = NULL; struct memtrack_handle *handle; @@ -56,22 +63,38 @@ static void memtrack_buffer_install_locked(struct rb_root *root, } else if (handle->buffer->id < buffer->id) { new = >rb_right; } else { - kref_get(>refcount); - return; + return handle; } } - handle = kmem_cache_alloc(memtrack_handle_cache, GFP_KERNEL); - if (!handle) - return; + if (alloc) { + handle = kmem_cache_alloc(memtrack_handle_cache, GFP_KERNEL); + if (!handle) + return NULL; - handle->buffer = buffer; - handle->root = root; - kref_init(>refcount); + handle->buffer = buffer; + handle->root = root; + kref_init(>refcount); + INIT_HLIST_HEAD(>vma_list); - rb_link_node(>node, parent, new); - rb_insert_color(>node, root); - atomic_inc(>buffer->userspace_handles); + rb_link_node(>node, parent, new); + rb_insert_color(>node, root); + atomic_inc(>buffer->userspace_handles); + } + + return NULL; +} + +static void memtrack_buffer_install_locked(struct rb_root *root, + struct memtrack_buffer *buffer) +{ + struct memtrack_handle *handle; + + handle = memtrack_handle_find_locked(root, buffer, true); + if (handle) { + kref_get(>refcount); + return; + } } /** @@ -112,19 +135,41 @@ static void memtrack_handle_destroy(struct kref *ref) static void memtrack_buffer_uninstall_locked(struct rb_root *root, struct memtrack_buffer *buffer) { - struct rb_node *node = root->rb_node; + struct memtrack_handle *handle; - while (node) { - struct memtrack_handle *handle = rb_entry(node, - struct memtrack_handle, node); + handle = memtrack_handle_find_locked(root, buffer, false); - if (handle->buffer->id > buffer->id) { - node = node->rb_left; - } else if (handle->buffer->id < buffer->id) { - node = node->rb_right; - } else { - kref_put(>refcount, memtrack_handle_destroy); - return; + if (handle) + kref_put(>refcount, memtrack_handle_destroy); +} + +static void memtrack_buffer_vm_open_locked(struct rb_root *root, + struct memtrack_buffer *buffer, + struct memtrack_vma_list *vma_list) +{ + struct memtrack_handle *handle; + + handle = memtrack_handle_find_locked(root, buffer, false); + if (handle) + hlist_add_head(_list->node, >vma_list); +} + +static void memtrack_buffer_vm_close_locked(struct rb_root *root, + struct memtrack_buffer *buffer, + const struct vm_area_struct *vma) +{ + struct memtrack_handle *handle; + + handle = memtrack_handle_find_locked(root, buffer, false); + if (handle) { + struct memtrack_vma_list *vma_list; + + hlist_for_each_entry(vma_list, >vma_list, node) { + if (vma_list->vma == vma) { + hlist_del(_list->node); + kfree(vma_list); + return; + } } } } @@ -153,6
[RFC 1/6] fs: add installed and uninstalled file_operations
These optional file_operations notify a file implementation when it is installed or uninstalled from a task's fd table. This can be used for accounting of file-backed shared resources like dma-buf. This involves some changes to the __fd_install() and __close_fd() APIs to actually pass along the responsible task_struct. These are low-level APIs with only two in-tree callers, both adjusted in this patch. Signed-off-by: Greg HackmannSigned-off-by: Ruchi Kandoi --- drivers/android/binder.c | 4 ++-- fs/file.c| 38 +- fs/open.c| 2 +- include/linux/fdtable.h | 4 ++-- include/linux/fs.h | 2 ++ 5 files changed, 36 insertions(+), 14 deletions(-) diff --git a/drivers/android/binder.c b/drivers/android/binder.c index 562af94..0bb174e 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -398,7 +398,7 @@ static void task_fd_install( struct binder_proc *proc, unsigned int fd, struct file *file) { if (proc->files) - __fd_install(proc->files, fd, file); + __fd_install(proc->tsk, fd, file); } /* @@ -411,7 +411,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) if (proc->files == NULL) return -ESRCH; - retval = __close_fd(proc->files, fd); + retval = __close_fd(proc->tsk, fd); /* can't restart close syscall because file table entry was cleared */ if (unlikely(retval == -ERESTARTSYS || retval == -ERESTARTNOINTR || diff --git a/fs/file.c b/fs/file.c index 69d6990..19c5fad 100644 --- a/fs/file.c +++ b/fs/file.c @@ -282,6 +282,24 @@ static unsigned int count_open_files(struct fdtable *fdt) return i; } +static inline void fdt_install(struct fdtable *fdt, int fd, struct file *file, + struct task_struct *task) +{ + if (file->f_op->installed) + file->f_op->installed(file, task); + rcu_assign_pointer(fdt->fd[fd], file); +} + +static inline void fdt_uninstall(struct fdtable *fdt, int fd, + struct task_struct *task) +{ + struct file *old_file = fdt->fd[fd]; + + if (old_file->f_op->uninstalled) + old_file->f_op->uninstalled(old_file, task); + rcu_assign_pointer(fdt->fd[fd], NULL); +} + /* * Allocate a new files structure and copy contents from the * passed in files structure. @@ -543,7 +561,7 @@ int __alloc_fd(struct files_struct *files, /* Sanity check */ if (rcu_access_pointer(fdt->fd[fd]) != NULL) { printk(KERN_WARNING "alloc_fd: slot %d not NULL!\n", fd); - rcu_assign_pointer(fdt->fd[fd], NULL); + fdt_uninstall(fdt, fd, current); } #endif @@ -601,10 +619,11 @@ EXPORT_SYMBOL(put_unused_fd); * fd_install() instead. */ -void __fd_install(struct files_struct *files, unsigned int fd, +void __fd_install(struct task_struct *task, unsigned int fd, struct file *file) { struct fdtable *fdt; + struct files_struct *files = task->files; might_sleep(); rcu_read_lock_sched(); @@ -618,13 +637,13 @@ void __fd_install(struct files_struct *files, unsigned int fd, smp_rmb(); fdt = rcu_dereference_sched(files->fdt); BUG_ON(fdt->fd[fd] != NULL); - rcu_assign_pointer(fdt->fd[fd], file); + fdt_install(fdt, fd, file, task); rcu_read_unlock_sched(); } void fd_install(unsigned int fd, struct file *file) { - __fd_install(current->files, fd, file); + __fd_install(current, fd, file); } EXPORT_SYMBOL(fd_install); @@ -632,10 +651,11 @@ EXPORT_SYMBOL(fd_install); /* * The same warnings as for __alloc_fd()/__fd_install() apply here... */ -int __close_fd(struct files_struct *files, unsigned fd) +int __close_fd(struct task_struct *task, unsigned fd) { struct file *file; struct fdtable *fdt; + struct files_struct *files = task->files; spin_lock(>file_lock); fdt = files_fdtable(files); @@ -644,7 +664,7 @@ int __close_fd(struct files_struct *files, unsigned fd) file = fdt->fd[fd]; if (!file) goto out_unlock; - rcu_assign_pointer(fdt->fd[fd], NULL); + fdt_uninstall(fdt, fd, task); __clear_close_on_exec(fd, fdt); __put_unused_fd(files, fd); spin_unlock(>file_lock); @@ -679,7 +699,7 @@ void do_close_on_exec(struct files_struct *files) file = fdt->fd[fd]; if (!file) continue; - rcu_assign_pointer(fdt->fd[fd], NULL); + fdt_uninstall(fdt, fd, current); __put_unused_fd(files, fd); spin_unlock(>file_lock); filp_close(file, files); @@ -846,7 +866,7 @@ __releases(>file_lock) if
[RFC 6/6] drivers: staging: ion: add ION_IOC_TAG ioctl
From: Greg HackmannION_IOC_TAG provides a userspace interface for tagging buffers with their memtrack usage after allocation. Signed-off-by: Ruchi Kandoi --- drivers/staging/android/ion/ion-ioctl.c | 17 + drivers/staging/android/uapi/ion.h | 25 + 2 files changed, 42 insertions(+) diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 7e7431d..8745a85 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -28,6 +28,7 @@ union ion_ioctl_arg { struct ion_handle_data handle; struct ion_custom_data custom; struct ion_heap_query query; + struct ion_tag_data tag; }; static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) @@ -162,6 +163,22 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) case ION_IOC_HEAP_QUERY: ret = ion_query_heaps(client, ); break; + case ION_IOC_TAG: + { +#ifdef CONFIG_MEMTRACK + struct ion_handle *handle; + + handle = ion_handle_get_by_id(client, data.tag.handle); + if (IS_ERR(handle)) + return PTR_ERR(handle); + data.tag.tag[sizeof(data.tag.tag) - 1] = 0; + memtrack_buffer_set_tag(>buffer->memtrack_buffer, + data.tag.tag); +#else + ret = -ENOTTY; +#endif + break; + } default: return -ENOTTY; } diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 14cd873..4c26196 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -115,6 +115,22 @@ struct ion_handle_data { ion_user_handle_t handle; }; +#define ION_MAX_TAG_LEN 32 + +/** + * struct ion_fd_data - metadata passed from userspace for a handle + * @handle:a handle + * @tag: a string describing the buffer + * + * For ION_IOC_TAG userspace populates the handle field with + * the handle returned from ion alloc and type contains the memtrack_type which + * accurately describes the usage for the memory. + */ +struct ion_tag_data { + ion_user_handle_t handle; + char tag[ION_MAX_TAG_LEN]; +}; + /** * struct ion_custom_data - metadata passed to/from userspace for a custom ioctl * @cmd: the custom ioctl function to call @@ -217,6 +233,15 @@ struct ion_heap_query { #define ION_IOC_SYNC _IOWR(ION_IOC_MAGIC, 7, struct ion_fd_data) /** + * DOC: ION_IOC_TAG - adds a memtrack descriptor tag to memory + * + * Takes an ion_tag_data struct with the type field populated with a + * memtrack_type and handle populated with a valid opaque handle. The + * memtrack_type should accurately define the usage for the memory. + */ +#define ION_IOC_TAG_IOWR(ION_IOC_MAGIC, 8, struct ion_tag_data) + +/** * DOC: ION_IOC_CUSTOM - call architecture specific ion ioctl * * Takes the argument of the architecture specific ioctl to call and -- 2.8.0.rc3.226.g39d4020 -- 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
[RFC 0/6] Module for tracking/accounting shared memory buffers
This patchstack introduces a new "memtrack" module for tracking and accounting memory exported to userspace as shared buffers, like dma-buf fds or GEM handles. Any process holding a reference to these buffers will keep the kernel from reclaiming its backing pages. mm counters don't provide a complete picture of these allocations, since they only account for pages that are mapped into a process's address space. This problem is especially bad for systems like Android that use dma-buf fds to share graphics and multimedia buffers between processes: these allocations are often large, have complex sharing patterns, and are rarely mapped into every process that holds a reference to them. memtrack maintains a per-process list of shared buffer references, which is exported to userspace as /proc/[pid]/memtrack. Buffers can be optionally "tagged" with a short string: for example, Android userspace would use this tag to identify whether buffers were allocated on behalf of the camera stack, GL, etc. memtrack also exports the VMAs associated with these buffers so that pages already included in the process's mm counters aren't double-counted. Shared-buffer allocators can hook into memtrack by embedding struct memtrack_buffer in their buffer metadata, calling memtrack_buffer_{init,remove} at buffer allocation and free time, and memtrack_buffer_{install,uninstall} when a userspace process takes or drops a reference to the buffer. For fd-backed buffers like dma-bufs, hooks in fdtable.c and fork.c automatically notify memtrack when references are added or removed from a process's fd table. This patchstack adds memtrack hooks into dma-buf and ion. If there's upstream interest in memtrack, it can be extended to other memory allocators as well, such as GEM implementations. Greg Hackmann (1): drivers: staging: ion: add ION_IOC_TAG ioctl Ruchi Kandoi (5): fs: add installed and uninstalled file_operations drivers: misc: add memtrack dma-buf: add memtrack support memtrack: Adds the accounting to keep track of all mmaped/unmapped pages. memtrack: Add memtrack accounting for forked processes. drivers/android/binder.c| 4 +- drivers/dma-buf/dma-buf.c | 37 +++ drivers/misc/Kconfig| 16 + drivers/misc/Makefile | 1 + drivers/misc/memtrack.c | 516 drivers/staging/android/ion/ion-ioctl.c | 17 ++ drivers/staging/android/ion/ion.c | 60 +++- drivers/staging/android/ion/ion_priv.h | 2 + drivers/staging/android/uapi/ion.h | 25 ++ fs/file.c | 38 ++- fs/open.c | 2 +- fs/proc/base.c | 4 + include/linux/dma-buf.h | 5 + include/linux/fdtable.h | 4 +- include/linux/fs.h | 2 + include/linux/memtrack.h| 130 include/linux/mm.h | 3 + include/linux/sched.h | 3 + kernel/fork.c | 23 +- 19 files changed, 875 insertions(+), 17 deletions(-) create mode 100644 drivers/misc/memtrack.c create mode 100644 include/linux/memtrack.h -- 2.8.0.rc3.226.g39d4020 -- 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 v2 28/31] cpia2_usb: don't use stack for DMA
Hi, > The USB control messages require DMA to work. We cannot pass > a stack-allocated buffer, as it is not warranted that the > stack would be into a DMA enabled area. > > Signed-off-by: Mauro Carvalho Chehab> Signed-off-by: Mauro Carvalho Chehab > --- > drivers/media/usb/cpia2/cpia2_usb.c | 32 +--- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/cpia2/cpia2_usb.c > b/drivers/media/usb/cpia2/cpia2_usb.c > index 13620cdf0599..417d683b237d 100644 > --- a/drivers/media/usb/cpia2/cpia2_usb.c > +++ b/drivers/media/usb/cpia2/cpia2_usb.c > @@ -545,10 +545,19 @@ static void free_sbufs(struct camera_data *cam) > static int write_packet(struct usb_device *udev, > u8 request, u8 * registers, u16 start, size_t size) > { > + unsigned char *buf; > + int ret; > + > if (!registers || size <= 0) > return -EINVAL; > > - return usb_control_msg(udev, > + buf = kmalloc(size, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + memcpy(buf, registers, size); > + > + ret = usb_control_msg(udev, > usb_sndctrlpipe(udev, 0), > request, > USB_TYPE_VENDOR | USB_RECIP_DEVICE, > @@ -557,6 +566,9 @@ static int write_packet(struct usb_device *udev, > registers, /* buffer */ = I think you also want to change the argument to usb_control_msg() from "registers" to "buf" in write_packet(). > size, > HZ); > + > + kfree(buf); > + return ret; > } .. --- Kosuke TATSUKAWA | 1st Platform Software Division | NEC Solution Innovators | ta...@ab.jp.nec.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: [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169
Em Tue, 11 Oct 2016 15:16:24 +0200 (CEST) Julia Lawall <julia.law...@lip6.fr> escreveu: > On Tue, 11 Oct 2016, Julia Lawall wrote: > > > It looks like a lock may be needed before line 174. > > Sorry, an unlock. I suspect that this is a false positive warning, as there is a mutex unlock on the same routine, at line 203. All exit conditions go to the unlock condition. Am I missing something? > > > > > julia > > > > -- Forwarded message -- > > Date: Tue, 11 Oct 2016 21:06:18 +0800 > > From: kbuild test robot <fengguang...@intel.com> > > To: kbu...@01.org > > Cc: Julia Lawall <julia.law...@lip6.fr> > > Subject: > > > > [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-medi > > a-usb-drivers/20161011-182408 3/31] > > drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on > > line > > 169 > > > > CC: kbuild-...@01.org > > TO: Mauro Carvalho Chehab <m.che...@samsung.com> > > CC: linux-media@vger.kernel.org > > CC: 0day robot <fengguang...@intel.com> > > > > tree: https://github.com/0day-ci/linux > > Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 > > head: ff49f775552fe4ebe2944527cf882073679cb1e5 > > commit: b38d98275e144aaea9db69ba2dcba58466046d9b [3/31] cinergyT2-core: > > handle error code on RC query > > :: branch date: 3 hours ago > > :: commit date: 3 hours ago > > > > >> drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on > > >> line 169 > > > > git remote add linux-review https://github.com/0day-ci/linux > > git remote update linux-review > > git checkout b38d98275e144aaea9db69ba2dcba58466046d9b > > vim +174 drivers/media/usb/dvb-usb/cinergyT2-core.c > > > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > 2008-09-19 163 { > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > 2008-09-19 164 struct cinergyt2_state *st = d->priv; > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 165 int i, ret; > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > 2008-09-19 166 > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > 2008-09-19 167 *state = REMOTE_NO_KEY_PRESSED; > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > 2008-09-19 168 > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 @169 mutex_lock(>data_mutex); > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 170 st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 171 > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 172 ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 173 if (ret < 0) > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 @174 return ret; > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 175 > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > 2016-10-11 176 if (st->data[4] == 0xff) { > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > 2008-09-19 177 /* key repeat */ > > > > --- > > 0-DAY kernel test infrastructureOpen Source Technology > > Center > > https://lists.01.org/pipermail/kbuild-all Intel > > Corporation > > > > -- Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169
On Tue, 11 Oct 2016, Mauro Carvalho Chehab wrote: > Em Tue, 11 Oct 2016 15:16:24 +0200 (CEST) > Julia Lawall <julia.law...@lip6.fr> escreveu: > > > On Tue, 11 Oct 2016, Julia Lawall wrote: > > > > > It looks like a lock may be needed before line 174. > > > > Sorry, an unlock. > > I suspect that this is a false positive warning, as there is a > mutex unlock on the same routine, at line 203. All exit > conditions go to the unlock condition. There is a direct exit in line 174. julia > > Am I missing something? > > > > > > > > > julia > > > > > > -- Forwarded message -- > > > Date: Tue, 11 Oct 2016 21:06:18 +0800 > > > From: kbuild test robot <fengguang...@intel.com> > > > To: kbu...@01.org > > > Cc: Julia Lawall <julia.law...@lip6.fr> > > > Subject: > > > > > > [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-medi > > > a-usb-drivers/20161011-182408 3/31] > > > drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on > > > line > > > 169 > > > > > > CC: kbuild-...@01.org > > > TO: Mauro Carvalho Chehab <m.che...@samsung.com> > > > CC: linux-media@vger.kernel.org > > > CC: 0day robot <fengguang...@intel.com> > > > > > > tree: https://github.com/0day-ci/linux > > > Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 > > > head: ff49f775552fe4ebe2944527cf882073679cb1e5 > > > commit: b38d98275e144aaea9db69ba2dcba58466046d9b [3/31] cinergyT2-core: > > > handle error code on RC query > > > :: branch date: 3 hours ago > > > :: commit date: 3 hours ago > > > > > > >> drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on > > > >> line 169 > > > > > > git remote add linux-review https://github.com/0day-ci/linux > > > git remote update linux-review > > > git checkout b38d98275e144aaea9db69ba2dcba58466046d9b > > > vim +174 drivers/media/usb/dvb-usb/cinergyT2-core.c > > > > > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > > 2008-09-19 163 { > > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > > 2008-09-19 164 struct cinergyt2_state *st = d->priv; > > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 165 int i, ret; > > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > > 2008-09-19 166 > > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > > 2008-09-19 167 *state = REMOTE_NO_KEY_PRESSED; > > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > > > 2008-09-19 168 > > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 @169 mutex_lock(>data_mutex); > > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 170 st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; > > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 171 > > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 172 ret = dvb_usb_generic_rw(d, st->data, 1, > > > st->data, 5, 0); > > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 173 if (ret < 0) > > > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 @174 return ret; > > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 175 > > > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > > > 2016-10-11 176 if (st->data[4] == 0xff) { > > > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > > > 2008-09-19 177 /* key repeat */ > > > > > > --- > > > 0-DAY kernel test infrastructureOpen Source Technology > > > Center > > > https://lists.01.org/pipermail/kbuild-all Intel > > > Corporation > > > > > > > > > > -- > Thanks, > Mauro > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 10:02:43PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkeywrote: Hi, On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey wrote: Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. Sorry I really don't follow. Which property? CRTC_ID? Userspace changing CRTC_ID will change connector_state->crtc (before we even get to a driver callback). After that, drm_atomic_helper_check_modeset derives connectors_changed based on the ->crtc pointers. After that, my encoder ->atomic_check *could* clear connectors_changed (or I could achieve the same thing by wrapping drm_atomic_helper_check), but it seems wrong to do so, considering that the connector routing *has* changed. If you think changing CRTC_ID shouldn't require a full modeset, I'd rather give drivers a ->needs_modeset callback to override the default drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into returning false. The problem with just that is that there's lots of different things that can feed into the overall needs_modeset variable. That's why we split it up into multiple booleans. So yes you're supposed to clear connectors_changed if there is some change that you can handle without a full modeset. If you want, think of connectors_changed as needs_modeset_due_to_change_in_connnector_state, but that's cumbersome to type and too long ;-) All right, got it :-). This intention wasn't clear to me from the comments in the code. I can imagine some hardware will need a full modeset to changed the writeback CRTC binding anyway. Yup, and then they can upgrade this again. With all these flow-control booleans the idea is very much that helpers give a default that works for 90% of all cases, and driver callbacks can then change it for the other 10%. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. I'm a little confused - what bit exactly is inconsistent? Not being truthful for when you need a modeset and when not. My implementation here is consistent with other connectors. Updating the writeback connector CRTC_ID
Re: [PATCH 2/6] ipr: use pci_irq_allocate_vectors
> "Christoph" == Christoph Hellwigwrites: Christoph> Switch the ipr driver to use pci_alloc_irq_vectors. We need Christoph> to two calls to pci_alloc_irq_vectors as ipr only supports Christoph> multiple MSI-X vectors, but not multiple MSI vectors. Christoph> Otherwise this cleans up a lot of cruft and allows to use a Christoph> common request_irq loop for irq types, which happens to only Christoph> iterate over a single line in the non MSI-X case. Applied to 4.10/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering -- 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/6] arcmsr: use pci_alloc_irq_vectors
> "Christoph" == Christoph Hellwigwrites: Christoph> Switch the arcmsr driver to use pci_alloc_irq_vectors. We Christoph> need to two calls to pci_alloc_irq_vectors as arcmsr only Christoph> supports multiple MSI-X vectors, but not multiple MSI Christoph> vectors. Christoph> Otherwise this cleans up a lot of cruft and allows to use a Christoph> common request_irq loop for irq types, which happens to only Christoph> iterate over a single line in the non MSI-X case. Applied to 4.10/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering -- 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: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 9:44 PM, Brian Starkeywrote: > Hi, > > > On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: >> >> On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkey >> wrote: >>> >>> Hi Daniel, >>> >>> Firstly thanks very much for having a look. >>> >>> >>> On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > > > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. >>> >>> Hmm, maybe. I don't think it's ideal - the driver would need to >>> re-implement drm_atomic_helper_check_modeset, which is quite a chunk >>> of code (along with exposing update_connector_routing, mode_fixup, >>> maybe others), and even after that it would have to lie and set >>> crtc_state->connectors_changed to false so that >>> drm_crtc_needs_modeset returns false to drm_atomic_check_only. >> >> >> You only need to update the property in your encoders's ->atomic_check >> function. No need for more, and I think being consistent with >> computing when you need a modeset is really a crucial part of the >> atomic ioctl that we should imo try to implement correctly as much as >> possible. >> > > Sorry I really don't follow. Which property? CRTC_ID? > > Userspace changing CRTC_ID will change connector_state->crtc (before > we even get to a driver callback). > > After that, drm_atomic_helper_check_modeset derives connectors_changed > based on the ->crtc pointers. > > After that, my encoder ->atomic_check *could* clear > connectors_changed (or I could achieve the same thing by wrapping > drm_atomic_helper_check), but it seems wrong to do so, considering > that the connector routing *has* changed. > > If you think changing CRTC_ID shouldn't require a full modeset, I'd > rather give drivers a ->needs_modeset callback to override the default > drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into > returning false. The problem with just that is that there's lots of different things that can feed into the overall needs_modeset variable. That's why we split it up into multiple booleans. So yes you're supposed to clear connectors_changed if there is some change that you can handle without a full modeset. If you want, think of connectors_changed as needs_modeset_due_to_change_in_connnector_state, but that's cumbersome to type and too long ;-) > I can imagine some hardware will need a full modeset to changed the > writeback CRTC binding anyway. Yup, and then they can upgrade this again. With all these flow-control booleans the idea is very much that helpers give a default that works for 90% of all cases, and driver callbacks can then change it for the other 10%. >>> I tried to keep special-casing of writeback connectors in the core to >>> a bare minimum, because I think it will quickly get messy and fragile >>> otherwise. >> >> >> Please always make the disdinction between core and shared drm >> helpers. Special cases in core == probably not good. Special cases in >> helpers == perfectly fine imo. >> >>> Honestly, I don't see modesetting the writeback connectors at >>> start-of-day as a big problem. >> >> >> It's inconsistent. Claiming it needs a modeset when it doesn't isn't >> great. Making that more discoverable to userspace is the entire point >> of atomic. And there might be hw where reconfiguring for writeback >> might need a full modeset. >> > > I'm a
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi, On Tue, Oct 11, 2016 at 07:01:33PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkeywrote: Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. Sorry I really don't follow. Which property? CRTC_ID? Userspace changing CRTC_ID will change connector_state->crtc (before we even get to a driver callback). After that, drm_atomic_helper_check_modeset derives connectors_changed based on the ->crtc pointers. After that, my encoder ->atomic_check *could* clear connectors_changed (or I could achieve the same thing by wrapping drm_atomic_helper_check), but it seems wrong to do so, considering that the connector routing *has* changed. If you think changing CRTC_ID shouldn't require a full modeset, I'd rather give drivers a ->needs_modeset callback to override the default drm_atomic_crtc_needs_modeset behaviour, instead of "tricking" it into returning false. I can imagine some hardware will need a full modeset to changed the writeback CRTC binding anyway. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. I'm a little confused - what bit exactly is inconsistent? My implementation here is consistent with other connectors. Updating the writeback connector CRTC_ID property requires a full modeset, the same as other connectors. Changing the FB_ID does *not* require a full modeset, because our hardware has no such restriction. This is analogous to updating the FB_ID on our planes, and is consistent with the other instances of the FB_ID property. If there is hardware which does have a restriction on changing FB_ID, I think that driver must be responsible for handling it in the same way as drivers which can't handle plane updates without a full modeset. Are you saying that because setting CRTC_ID on Mali-DP is a no-op, it shouldn't require a full modeset? I'd rather somehow hard-code the CRTC_ID for our writeback connector to have it always attached to the CRTC in that case. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode
Re: [RFC PATCH 00/11] Introduce writeback connectors
Brian Starkeywrites: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. > > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. > > Writeback can be disabled by setting FB_ID to zero. I think this sounds great, and the interface is just right IMO. I don't really see a use for continuous mode -- a sequence of one-shots makes a lot more sense because then you can know what data has changed, which anyone trying to use the writeback buffer would need to know. > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. My hardware won't necessarily finish by the next vsync -- it trickles out at whatever rate it can find memory bandwidth to get the job done, and fires an interrupt when it's finished. So I would like some definition for how syncing works. One answer would be that these flips don't trigger their pageflip events until the writeback is done (so I need to collect both the vsync irq and the writeback irq before sending). Another would be that manage an independent fence for the writeback fb, so that you still immediately know when framebuffers from the previous scanout-only frame are idle. Also, tests for this in igt, please. Writeback in igt will give us so much more ability to cover KMS functionality on non-Intel hardware. signature.asc Description: PGP signature
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 6:43 PM, Brian Starkeywrote: > Hi Daniel, > > Firstly thanks very much for having a look. > > > On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: >> >> On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: >>> >>> Hi, >>> >>> This RFC series introduces a new connector type: >>> DRM_MODE_CONNECTOR_WRITEBACK >>> It is a follow-on from a previous discussion: [1] >>> >>> Writeback connectors are used to expose the memory writeback engines >>> found in some display controllers, which can write a CRTC's >>> composition result to a memory buffer. >>> This is useful e.g. for testing, screen-recording, screenshots, >>> wireless display, display cloning, memory-to-memory composition. >>> >>> Patches 1-7 include the core framework changes required, and patches >>> 8-11 implement a writeback connector for the Mali-DP writeback engine. >>> The Mali-DP patches depend on this other series: [2]. >>> >>> The connector is given the FB_ID property for the output framebuffer, >>> and two new read-only properties: PIXEL_FORMATS and >>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel >>> formats of the engine. >>> >>> The EDID property is not exposed for writeback connectors. >>> >>> Writeback connector usage: >>> -- >>> Due to connector routing changes being treated as "full modeset" >>> operations, any client which wishes to use a writeback connector >>> should include the connector in every modeset. The writeback will not >>> actually become active until a framebuffer is attached. >> >> >> Erhm, this is just the default, drivers can override this. And we could >> change the atomic helpers to not mark a modeset as a modeset if the >> connector that changed is a writeback one. >> > > Hmm, maybe. I don't think it's ideal - the driver would need to > re-implement drm_atomic_helper_check_modeset, which is quite a chunk > of code (along with exposing update_connector_routing, mode_fixup, > maybe others), and even after that it would have to lie and set > crtc_state->connectors_changed to false so that > drm_crtc_needs_modeset returns false to drm_atomic_check_only. You only need to update the property in your encoders's ->atomic_check function. No need for more, and I think being consistent with computing when you need a modeset is really a crucial part of the atomic ioctl that we should imo try to implement correctly as much as possible. > I tried to keep special-casing of writeback connectors in the core to > a bare minimum, because I think it will quickly get messy and fragile > otherwise. Please always make the disdinction between core and shared drm helpers. Special cases in core == probably not good. Special cases in helpers == perfectly fine imo. > Honestly, I don't see modesetting the writeback connectors at > start-of-day as a big problem. It's inconsistent. Claiming it needs a modeset when it doesn't isn't great. Making that more discoverable to userspace is the entire point of atomic. And there might be hw where reconfiguring for writeback might need a full modeset. >>> The writeback itself is enabled by attaching a framebuffer to the >>> FB_ID property of the connector. The driver must then ensure that the >>> CRTC content of that atomic commit is written into the framebuffer. >>> >>> The writeback works in a one-shot mode with each atomic commit. This >>> prevents the same content from being written multiple times. >>> In some cases (front-buffer rendering) there might be a desire for >>> continuous operation - I think a property could be added later for >>> this kind of control. >>> >>> Writeback can be disabled by setting FB_ID to zero. >> >> >> This seems to contradict itself: If it's one-shot, there's no need to >> disable it - it will auto-disable. > > > I should have explained one-shot more clearly. What I mean is, one > drmModeAtomicCommit == one write to memory. This is as opposed to > writing the same thing to memory every vsync until it is stopped > (which our HW is capable of doing). > > A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID > will write (again - but with whatever scene updates) to the same > framebuffer. > > This continues for every drmModeAtomicCommit until FB_ID is set to > zero - to disable writing - or changed to a different framebuffer, in > which case we write to the new one. > > IMO this behaviour makes sense in the context of the rest of Atomic, > and as the FB_ID is indeed persistent across atomic commits, I think > it should be read-able. tbh I don't like that, I think it'd be better to make this truly one-shot. Otherwise we'll have real fun problems with hw where the writeback can take longer than a vblank (it happens ...). So one-shot, with auto-clearing to NULL/0 is imo the right approach. >> In other cases where we write a property as a one-shot thing (fences for >> android). In that case when you read that property it's always 0 (well,
Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
On Tue, Oct 11, 2016 at 6:47 PM, Brian Starkeywrote: > On Tue, Oct 11, 2016 at 05:44:48PM +0200, Daniel Vetter wrote: >> >> On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote: >>> >>> Writeback connectors aren't much use to the fbdev helpers, as they won't >>> show anything to the user. Skip them when looking for candidate output >>> configurations. >>> >>> Signed-off-by: Brian Starkey >>> --- >>> drivers/gpu/drm/drm_fb_helper.c |4 >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_fb_helper.c >>> b/drivers/gpu/drm/drm_fb_helper.c >>> index 03414bd..dedf6e7 100644 >>> --- a/drivers/gpu/drm/drm_fb_helper.c >>> +++ b/drivers/gpu/drm/drm_fb_helper.c >>> @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper >>> *fb_helper, >>> if (modes[n] == NULL) >>> return best_score; >>> >>> + /* Writeback connectors aren't much use for fbdev */ >>> + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) >>> + return best_score; >> >> >> I think we could handle this by always marking writeback connectors as >> disconnected. Userspace and fbdev emulation should then avoid them, >> always. >> -Daniel >> > > Good idea; I'll need to take a closer look at how it would interact > with the probe helper (connector->force etc). > > Are you thinking instead-of or in-addition-to the client cap? I'd be > worried about apps doing strange things and trying to use even > disconnected connectors. Apps shouldn't try to use disconnected connectors, at least by default. I think we wouldn't need the cap in that case. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
On Tue, Oct 11, 2016 at 05:44:48PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote: Writeback connectors aren't much use to the fbdev helpers, as they won't show anything to the user. Skip them when looking for candidate output configurations. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_fb_helper.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bd..dedf6e7 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, if (modes[n] == NULL) return best_score; + /* Writeback connectors aren't much use for fbdev */ + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) + return best_score; I think we could handle this by always marking writeback connectors as disconnected. Userspace and fbdev emulation should then avoid them, always. -Daniel Good idea; I'll need to take a closer look at how it would interact with the probe helper (connector->force etc). Are you thinking instead-of or in-addition-to the client cap? I'd be worried about apps doing strange things and trying to use even disconnected connectors. + crtcs = kzalloc(fb_helper->connector_count * sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); if (!crtcs) -- 1.7.9.5 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- 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: [RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic
On Tue, Oct 11, 2016 at 03:54:01PM +0100, Brian Starkey wrote: > Implement the CRTC/Plane disable functionality of drm_framebuffer_remove > using the atomic API, and use it if possible. > > For atomic drivers, this removes the possibility of several commits when > a framebuffer is in use by more than one CRTC/plane. > > Additionally, this will provide a suitable place to support the removal > of a framebuffer from a writeback connector, in the case that a > writeback connector is still actively using a framebuffer when it is > removed by userspace. > > Signed-off-by: Brian StarkeyJust the small comment here: Last time around I wanted toland an atomic disable function for fb remove code it blew up. Need to check out git history to make sure we've addressed those isses. Caveat emperor ;-) -Daniel > --- > drivers/gpu/drm/drm_framebuffer.c | 154 > - > 1 file changed, 152 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c > b/drivers/gpu/drm/drm_framebuffer.c > index 528f75d..b02cf73 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -22,6 +22,7 @@ > > #include > #include > +#include > #include > #include > > @@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) > EXPORT_SYMBOL(drm_framebuffer_cleanup); > > /** > + * __drm_framebuffer_remove_atomic - atomic version of > __drm_framebuffer_remove > + * @dev: drm device > + * @fb: framebuffer to remove > + * > + * If the driver implements the atomic API, we can handle the disabling of > all > + * CRTCs/planes which use a framebuffer which is going away in a single > atomic > + * commit. > + * > + * This scans all CRTCs and planes in @dev's mode_config. If they're using > @fb, > + * it is removed and the CRTC/plane disabled. > + * The legacy references are dropped and the ->fb pointers set to NULL > + * accordingly. > + * > + * Returns: > + * true if the framebuffer was successfully removed from use > + */ > +static bool __drm_framebuffer_remove_atomic(struct drm_device *dev, > + struct drm_framebuffer *fb) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector_state *conn_state; > + struct drm_connector *connector; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + unsigned plane_mask; > + int i, ret; > + > + drm_modeset_acquire_init(, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return false; > + > + state->acquire_ctx = > + > +retry: > + drm_for_each_crtc(crtc, dev) { > + struct drm_plane_state *primary_state; > + struct drm_crtc_state *crtc_state; > + > + primary_state = drm_atomic_get_plane_state(state, > crtc->primary); > + if (IS_ERR(primary_state)) { > + ret = PTR_ERR(primary_state); > + goto fail; > + } > + > + if (primary_state->fb != fb) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > + if (IS_ERR(crtc_state)) { > + ret = PTR_ERR(crtc_state); > + goto fail; > + } > + > + /* Only handle the CRTC itself here, handle the plane later */ > + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); > + if (ret != 0) > + goto fail; > + > + crtc_state->active = false; > + > + /* Get the connectors in order to disable them */ > + ret = drm_atomic_add_affected_connectors(state, crtc); > + if (ret) > + goto fail; > + } > + > + plane_mask = 0; > + drm_for_each_plane(plane, dev) { > + struct drm_plane_state *plane_state; > + > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) { > + ret = PTR_ERR(plane_state); > + goto fail; > + } > + > + if (plane_state->fb != fb) > + continue; > + > + plane->old_fb = plane->fb; > + plane_mask |= 1 << drm_plane_index(plane); > + > + /* > + * Open-coded copy of __drm_atomic_helper_disable_plane to avoid > + * a dependency on atomic-helper > + */ > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > + if (ret != 0) > + goto fail; > + > + drm_atomic_set_fb_for_plane(plane_state, NULL); > + plane_state->crtc_x = 0; > + plane_state->crtc_y = 0; > + plane_state->crtc_w = 0; > + plane_state->crtc_h = 0; > + plane_state->src_x = 0; > + plane_state->src_y = 0; > +
Re: [PATCH 0/4] reST-directive kernel-cmd / include contentent from scripts
Em Tue, 11 Oct 2016 18:06:46 +0200 Markus Heiserescreveu: > Am 11.10.2016 um 17:34 schrieb Jani Nikula : > > > On Tue, 11 Oct 2016, Mauro Carvalho Chehab wrote: > >> Em Tue, 11 Oct 2016 09:26:48 +0200 > >> Markus Heiser escreveu: > >> > >>> Am 07.10.2016 um 07:56 schrieb Jani Nikula : > >>> > On Thu, 06 Oct 2016, Mauro Carvalho Chehab > wrote: > > Em Thu, 06 Oct 2016 17:21:36 +0300 > > Jani Nikula escreveu: > >> We've seen what happens when we make it easy to add random scripts to > >> build documentation. We've worked hard to get rid of that. In my books, > >> one of the bigger points in favor of Sphinx over AsciiDoc(tor) was > >> getting rid of all the hacks required in the build. Things that broke > >> in > >> subtle ways. > > > > I really can't see what scripts it get rids. > > Really? You don't see why the DocBook build was so fragile and difficult > to maintain? That scares me a bit, because then you will not have > learned why we should at all costs avoid adding random scripts to > produce documentation. > >>> > >>> For me, disassembling the DocBok build was hard and bothersome, I don't > >>> want this back. > >>> > >>> IMO: old hats are productive with perl and they won't adapt another > >>> interpreter language (like python) for scripting. > >>> > >>> This series -- the kernel-cmd -- directive avoid that they build > >>> fragile and difficult to maintain Makefile constructs, calling their > >>> perl scripts. > >>> > >>> Am 06.10.2016 um 16:21 schrieb Jani Nikula : > >>> > This is connected to the above: keeping documentation buildable with > sphinx-build directly will force you to avoid the Makefile hacks. > >>> > >>> > >>> Thats why I think, that the kernel-cmd directive is a more *straight- > >>> forward* solution, helps to **avoid** complexity while not everyone > >>> has to script in python ... > >>> > Case in point, parse-headers.pl was added for a specific need of media > documentation, and for the life of me I can't figure out by reading the > script what good, if any, it would be for gpu documentation. I call > *that* unmaintainable. > >>> > >>> > >>> If one adds a script like parse-headers.pl to the Documentation/sphinx > >>> folder, he/she also has to add a documentation to the > >>> kernel-documentation.rst > >>> > >>> If the kernel-cmd directive gets acked, I will add a description to > >>> kernel-documentation.rst and I request Mauro to document the > >>> parse-headers.pl > >>> also. > >> > >> I can write documentation for parse-headers.pl, either as a --help/--man > >> option or at some ReST file (or both). I'll add this to my mental TODO > >> list. > > > > Thanks, documentation will help everyone else evaluate whether > > parse-headers.pl is only useful for some corner case in media docs, or > > perhaps more generally useful. Currently, it's hard to tell. > > > > Anyway, documentation does not change my view on adding such scripts. As > > I've said, I think they will make the documentation build more difficult > > to maintain. They are likely to become special purpose hacks for corner > > cases across documentation. > > Hmm, why not generating reST content by (Perl) scripts? From my POV, > the scripts/kernel-doc is such a script and parse-headers.pl > is just another. And Sphinx is a third one - or, actually, a set of scripts, as each extension is also a script ;) > The only difference of both is, that kernel-doc > has its own integration (directive) while kernel-cmd is a simple > solution to call scripts (e.g. parse-headers.pl) within sphinx-build. > > Joking: the kernel-doc directive could replaced by > the kernel-cmd directive ;-) I'm actually OK with either strategy - although it sounds easier to maintain if we have a single extension to run an external script - including kernel-doc - specially if the Sphinx extension API and/or Python changes in the future in a non-backward compatible way. > > The rest of what you say is unrelated to the patches at hand. > > I think Mauro want's to address your (justifiable) fear about > complexity and hacks. IMO he says (by examples); "there are a lot of > other hard to maintain hacks required, especially when it comes > to build PDF". > > IMO, complexity is not reduced by prohibit scripts, it is > ongoing job of the maintainers to observe. Yes, that's exactly my point. IMO, not allowing scripts actually increase complexity, as we'll need Makefile rules to generate rst files, if we won't allow doing that via perl/python scripts. If we allow such scripts (and we do since day zero, due to kernel-doc), then there are 3 options: 1) use a single python
Re: [RFC PATCH 00/11] Introduce writeback connectors
Hi Daniel, Firstly thanks very much for having a look. On Tue, Oct 11, 2016 at 05:43:59PM +0200, Daniel Vetter wrote: On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. Hmm, maybe. I don't think it's ideal - the driver would need to re-implement drm_atomic_helper_check_modeset, which is quite a chunk of code (along with exposing update_connector_routing, mode_fixup, maybe others), and even after that it would have to lie and set crtc_state->connectors_changed to false so that drm_crtc_needs_modeset returns false to drm_atomic_check_only. I tried to keep special-casing of writeback connectors in the core to a bare minimum, because I think it will quickly get messy and fragile otherwise. Honestly, I don't see modesetting the writeback connectors at start-of-day as a big problem. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. This seems to contradict itself: If it's one-shot, there's no need to disable it - it will auto-disable. I should have explained one-shot more clearly. What I mean is, one drmModeAtomicCommit == one write to memory. This is as opposed to writing the same thing to memory every vsync until it is stopped (which our HW is capable of doing). A subsequent drmModeAtomicCommit which doesn't touch the writeback FB_ID will write (again - but with whatever scene updates) to the same framebuffer. This continues for every drmModeAtomicCommit until FB_ID is set to zero - to disable writing - or changed to a different framebuffer, in which case we write to the new one. IMO this behaviour makes sense in the context of the rest of Atomic, and as the FB_ID is indeed persistent across atomic commits, I think it should be read-able. In other cases where we write a property as a one-shot thing (fences for android). In that case when you read that property it's always 0 (well, -1 for fences since file descriptor). That also avoids the issues when userspace unconditionally saves/restores all properties (this is needed for generic compositor switching). I think a better behaviour would be to do the same trick, with FB_ID on the connector always returning 0 as the current value. That encodes the one-shot behaviour directly. For one-shot vs continuous: Maybe we want to simply have a separate writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and FB_WRITEBACK_CONTINUOUS_ID. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the crtc. it disables everything, i.e. also writeback. So removing the DPMS property is a viable option for writeback connectors in your opinion? * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. > > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. I though people agreed that this sort of thing would go through v4l. Continously writing to the same buffer isn't perhaps all that sensible anyway, and so we'd need queueing, which is what v4l has already. Well, I guess we might add some queueing to atomic eventually? I guess for front buffer rendering type of thing you might have some use for a continuous mode targeting a single fb. Though I think peridically triggering a new write could do as well. Of course either way would likely tear horribly, and having multiple buffers seems like the better option. > > Writeback can be disabled by setting FB_ID to zero. > > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. > > See Also: > - > [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html > [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html > > I welcome any comments, especially if this approach does/doesn't fit > well with anyone else's hardware. > > Thanks, > > -Brian > > --- > > Brian Starkey (10): > drm: add writeback connector type > drm/fb-helper: skip writeback connectors > drm: extract CRTC/plane disable from drm_framebuffer_remove > drm: add __drm_framebuffer_remove_atomic > drm: add fb to connector state > drm: expose fb_id property for writeback connectors > drm: add writeback-connector pixel format properties > drm: mali-dp: rename malidp_input_format > drm: mali-dp: add RGB writeback formats for DP550/DP650 > drm: mali-dp: add writeback connector > > Liviu Dudau (1): > drm: mali-dp: Add support for writeback on DP550/DP650 > > drivers/gpu/drm/arm/Makefile|1 + > drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ > drivers/gpu/drm/arm/malidp_drv.c| 25 +++- > drivers/gpu/drm/arm/malidp_drv.h|5 + > drivers/gpu/drm/arm/malidp_hw.c | 104 ++ > drivers/gpu/drm/arm/malidp_hw.h | 27 +++- > drivers/gpu/drm/arm/malidp_mw.c | 268 > +++ > drivers/gpu/drm/arm/malidp_planes.c |8 +- > drivers/gpu/drm/arm/malidp_regs.h | 15 ++ > drivers/gpu/drm/drm_atomic.c| 40 ++ > drivers/gpu/drm/drm_atomic_helper.c |4 + > drivers/gpu/drm/drm_connector.c | 79 ++- > drivers/gpu/drm/drm_crtc.c | 14 +- > drivers/gpu/drm/drm_fb_helper.c |4 + > drivers/gpu/drm/drm_framebuffer.c
Re: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 6:25 PM, Ville Syrjäläwrote: >> Writeback connector usage: >> -- >> Due to connector routing changes being treated as "full modeset" >> operations, any client which wishes to use a writeback connector >> should include the connector in every modeset. The writeback will not >> actually become active until a framebuffer is attached. >> >> The writeback itself is enabled by attaching a framebuffer to the >> FB_ID property of the connector. The driver must then ensure that the >> CRTC content of that atomic commit is written into the framebuffer. >> >> The writeback works in a one-shot mode with each atomic commit. This >> prevents the same content from being written multiple times. >> In some cases (front-buffer rendering) there might be a desire for >> continuous operation - I think a property could be added later for >> this kind of control. > > I though people agreed that this sort of thing would go through v4l. > Continously writing to the same buffer isn't perhaps all that sensible > anyway, and so we'd need queueing, which is what v4l has already. Well, > I guess we might add some queueing to atomic eventually? > > I guess for front buffer rendering type of thing you might have some > use for a continuous mode targeting a single fb. Though I think > peridically triggering a new write could do as well. Of course either > way would likely tear horribly, and having multiple buffers seems like > the better option Yeah, momentarily entirely forgot about v4l. I think making FB_ID one-shot (perhaps better to call it WRITEBACK_FB_ID to avoid confusion) is the right thing to do, and then push everything continuous to some form of drm/v4l integration. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- 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
[GIT PULL for v4.9-rc1] media updates
Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media tags/media/v4.9-1 For media patches for Kernel 4.9: - Documentation improvements: conversion of all non-DocBook documents to Sphinx and lots of fixes to the uAPI media book; - New PCI driver for Techwell TW5864 media grabber boards; - New SoC driver for ATMEL Image Sensor Controller; - Removal of some obsolete SoC drivers (s5p-tv driver and soc_camera drivers); - Addition of ST CEC driver; - Lots of drivers fixes, improvements and additions; Regards, Mauro - You should expect a trivial conflict at the MAINTAINERS file, due to the ATMEL driver. PS.: This patchset has merges from Jon's docs tree, as several patches on it depends on the Sphinx build improvements that I sent to Jon to be merged via his tree. The diffstat generated via this command: git request-pull 02bafd96f3a5 git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media media/v4.9-1 looks weird, as it included also the changes that were already merged on your tree via Jonathan's documentation tree. I tested merging on the top of your master branch here and everything is OK. The following changes since commit c8d2bc9bc39ebea8437fd974fdbc21847bb897a3: Linux 4.8 (2016-10-02 16:24:33 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media tags/media/v4.9-1 for you to fetch changes up to 9fce0c226536fc36c7fb0a8ca38a995be43e: Merge tag 'v4.8' into patchwork (2016-10-05 16:43:53 -0300) media updates for v4.9-rc1 Abylay Ospan (4): [media] cxd2841er: freeze/unfreeze registers when reading stats [media] cxd2841er: BER and SNR reading for ISDB-T [media] cxd2841er: force 8MHz bandwidth for DVB-C if specified bw not supported [media] lgdt3306a: remove 20*50 msec unnecessary timeout Andrey Utkin (4): [media] pci: Add tw5864 driver [media] tw5864-core: remove excessive irqsave [media] tw5864: constify vb2_ops structure [media] tw5864: constify struct video_device template Antti Palosaari (9): [media] cxd2820r: improve IF frequency setting [media] cxd2820r: dvbv5 statistics for DVB-T [media] cxd2820r: dvbv5 statistics for DVB-T2 [media] cxd2820r: dvbv5 statistics for DVB-C [media] cxd2820r: wrap legacy DVBv3 statistics via DVBv5 statistics [media] cxd2820r: add I2C driver bindings [media] cxd2820r: correct logging [media] cxd2820r: improve lock detection [media] cxd2820r: convert to regmap api Arnd Bergmann (8): [media] pulse8-cec: avoid uninitialized data use [media] Input: atmel_mxt: disallow impossible configuration [media] Input: synaptics-rmi4: disallow impossible configuration [media] ad5820: use __maybe_unused for PM functions [media] atmel-isc: mark PM functions as __maybe_unused [media] usb: gadget: uvc: add V4L2 dependency [media] dvb-usb: split out common parts of dibusb [media] dvb-usb: avoid link error with dib3000m{b,c| Baoyou Xie (1): [media] staging: media: omap4iss: mark omap4iss_flush() static Benjamin Gaignard (4): [media] bindings for stih-cec driver [media] add stih-cec driver [media] add stih-cec driver into DT [media] add maintainer for stih-cec driver Bhaktipriya Shridhar (9): [media] pvrusb2: Remove deprecated create_singlethread_workqueue [media] gspca: sonixj: Remove deprecated create_singlethread_workqueue [media] gspca: vicam: Remove deprecated create_singlethread_workqueue [media] gspca: jl2005bcd: Remove deprecated create_singlethread_workqueue [media] gspca: finepix: Remove deprecated create_singlethread_workqueue [media] ad9389b: Remove deprecated create_singlethread_workqueue [media] s5p-mfc: Remove deprecated create_singlethread_workqueue [media] cx25821: Drop Freeing of Workqueue [media] cx25821: Remove deprecated create_singlethread_workqueue Charles-Antoine Couret (3): [media] SDI: add flag for SDI formats and SMPTE 125M definition [media] V4L2: Add documentation for SDI timings and related flags [media] Add GS1662 driver, a video serializer Christophe JAILLET (2): [media] drxd_hard: Add missing error code assignment before test [media] s5p-cec: Fix memory allocation failure check Colin Ian King (4): [media] helene: fix memory leak when heleno_x_pon fails [media] pxa_camera: fix spelling mistake: "dequeud" -> "dequeued" [media] rc/streamzap: fix spelling mistake "sumbiting" -> "submitting" [media] lgdt3306a: fix spelling mistake "supportted" -> "supported" Ezequiel Garcia (2): [media] media: tw686x: Rework initial hardware configuration [media] media: tw686x: Support frame sizes and frame
Re: [PATCH 0/4] reST-directive kernel-cmd / include contentent from scripts
Am 11.10.2016 um 17:34 schrieb Jani Nikula: > On Tue, 11 Oct 2016, Mauro Carvalho Chehab wrote: >> Em Tue, 11 Oct 2016 09:26:48 +0200 >> Markus Heiser escreveu: >> >>> Am 07.10.2016 um 07:56 schrieb Jani Nikula : >>> On Thu, 06 Oct 2016, Mauro Carvalho Chehab wrote: > Em Thu, 06 Oct 2016 17:21:36 +0300 > Jani Nikula escreveu: >> We've seen what happens when we make it easy to add random scripts to >> build documentation. We've worked hard to get rid of that. In my books, >> one of the bigger points in favor of Sphinx over AsciiDoc(tor) was >> getting rid of all the hacks required in the build. Things that broke in >> subtle ways. > > I really can't see what scripts it get rids. Really? You don't see why the DocBook build was so fragile and difficult to maintain? That scares me a bit, because then you will not have learned why we should at all costs avoid adding random scripts to produce documentation. >>> >>> For me, disassembling the DocBok build was hard and bothersome, I don't >>> want this back. >>> >>> IMO: old hats are productive with perl and they won't adapt another >>> interpreter language (like python) for scripting. >>> >>> This series -- the kernel-cmd -- directive avoid that they build >>> fragile and difficult to maintain Makefile constructs, calling their >>> perl scripts. >>> >>> Am 06.10.2016 um 16:21 schrieb Jani Nikula : >>> This is connected to the above: keeping documentation buildable with sphinx-build directly will force you to avoid the Makefile hacks. >>> >>> >>> Thats why I think, that the kernel-cmd directive is a more *straight- >>> forward* solution, helps to **avoid** complexity while not everyone >>> has to script in python ... >>> Case in point, parse-headers.pl was added for a specific need of media documentation, and for the life of me I can't figure out by reading the script what good, if any, it would be for gpu documentation. I call *that* unmaintainable. >>> >>> >>> If one adds a script like parse-headers.pl to the Documentation/sphinx >>> folder, he/she also has to add a documentation to the >>> kernel-documentation.rst >>> >>> If the kernel-cmd directive gets acked, I will add a description to >>> kernel-documentation.rst and I request Mauro to document the >>> parse-headers.pl >>> also. >> >> I can write documentation for parse-headers.pl, either as a --help/--man >> option or at some ReST file (or both). I'll add this to my mental TODO >> list. > > Thanks, documentation will help everyone else evaluate whether > parse-headers.pl is only useful for some corner case in media docs, or > perhaps more generally useful. Currently, it's hard to tell. > > Anyway, documentation does not change my view on adding such scripts. As > I've said, I think they will make the documentation build more difficult > to maintain. They are likely to become special purpose hacks for corner > cases across documentation. Hmm, why not generating reST content by (Perl) scripts? From my POV, the scripts/kernel-doc is such a script and parse-headers.pl is just another. The only difference of both is, that kernel-doc has its own integration (directive) while kernel-cmd is a simple solution to call scripts (e.g. parse-headers.pl) within sphinx-build. Joking: the kernel-doc directive could replaced by the kernel-cmd directive ;-) > The rest of what you say is unrelated to the patches at hand. I think Mauro want's to address your (justifiable) fear about complexity and hacks. IMO he says (by examples); "there are a lot of other hard to maintain hacks required, especially when it comes to build PDF". IMO, complexity is not reduced by prohibit scripts, it is ongoing job of the maintainers to observe. Anyway, these are only my 2cent. I'am interested in what Jon says in general about using (Perl) scripts to generate reST content. --Markus-- -- 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: [RFC PATCH 07/11] drm: Add writeback-connector pixel format properties
On Tue, Oct 11, 2016 at 03:54:04PM +0100, Brian Starkey wrote: > So that userspace can determine what pixel formats are supported for a > writeback connector's framebuffer, add a pixel format list to writeback > connectors. This is in the form of an immutable blob containing an array > of formats, and an immutable uint holding the array size. > > Signed-off-by: Brian StarkeyI think we should have a dedicated writeback property registration function, e.g. drm_writeback_connector_init(). That would then take the pixel format list and everything else and make sure it's set up correctly. For safety we might want to put a WARN_ON(type == WRITEBACK) into drm_connector_init, to make sure no one botches this up. Maybe even put all that into a new drm_writeback.c file, that then also gives you a nice place to group all the documentation (including the DOC: overview comment). > --- > drivers/gpu/drm/drm_connector.c | 73 > ++- > include/drm/drm_connector.h | 12 +++ > include/drm/drm_crtc.h | 12 +++ > 3 files changed, 96 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index fb83870..2f1f61d 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -249,9 +249,14 @@ int drm_connector_init(struct drm_device *dev, > drm_object_attach_property(>base, > config->prop_crtc_id, 0); > } > > - if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > + if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) { > drm_object_attach_property(>base, > config->prop_fb_id, 0); > + drm_object_attach_property(>base, > +config->pixel_formats_property, 0); > + drm_object_attach_property(>base, > +config->pixel_formats_size_property, > 0); > + } > > connector->debugfs_entry = NULL; > out_put_type_id: > @@ -851,6 +856,45 @@ int drm_mode_create_suggested_offset_properties(struct > drm_device *dev) > EXPORT_SYMBOL(drm_mode_create_suggested_offset_properties); > > /** > + * drm_mode_create_writeback_connector_properties - create writeback > connector properties > + * @dev: DRM device > + * > + * Create the properties specific to writeback connectors. These will be > attached > + * to writeback connectors by drm_connector_init. Drivers can set these > + * properties using drm_mode_connector_set_writeback_formats(). > + * > + * "PIXEL_FORMATS": > + * Immutable blob property to store the supported pixel formats table. The > + * data is an array of u32 DRM_FORMAT_* fourcc values. > + * Userspace can use this blob to find out what pixel formats are supported > + * by the connector's writeback engine. > + * > + * "PIXEL_FORMATS_SIZE": > + * Immutable unsigned range property storing the number of entries in > the > + * PIXEL_FORMATS array. > + */ > +int drm_mode_create_writeback_connector_properties(struct drm_device *dev) > +{ > + if (dev->mode_config.pixel_formats_property && > + dev->mode_config.pixel_formats_size_property) > + return 0; > + > + dev->mode_config.pixel_formats_property = > + drm_property_create(dev, DRM_MODE_PROP_BLOB | > DRM_MODE_PROP_IMMUTABLE, > + "PIXEL_FORMATS", 0); > + > + dev->mode_config.pixel_formats_size_property = > + drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, > + "PIXEL_FORMATS_SIZE", 0, UINT_MAX); > + > + if (dev->mode_config.pixel_formats_property == NULL || > + dev->mode_config.pixel_formats_size_property == NULL) > + return -ENOMEM; > + return 0; > +} > +EXPORT_SYMBOL(drm_mode_create_writeback_connector_properties); > + > +/** > * drm_mode_connector_set_path_property - set tile property on connector > * @connector: connector to set property on. > * @path: path to use for property; must not be NULL. > @@ -957,6 +1001,33 @@ int drm_mode_connector_update_edid_property(struct > drm_connector *connector, > } > EXPORT_SYMBOL(drm_mode_connector_update_edid_property); > > +int drm_mode_connector_set_writeback_formats(struct drm_connector *connector, > + u32 *formats, > + unsigned int n_formats) > +{ > + struct drm_device *dev = connector->dev; > + size_t size = n_formats * sizeof(*formats); > + int ret; > + > + if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) > + return -EINVAL; > + > + ret = drm_property_replace_global_blob(dev, > + > >pixel_formats_blob_ptr, > +size, > +formats, > +
Re: [RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
On Tue, Oct 11, 2016 at 03:53:59PM +0100, Brian Starkey wrote: > Writeback connectors aren't much use to the fbdev helpers, as they won't > show anything to the user. Skip them when looking for candidate output > configurations. > > Signed-off-by: Brian Starkey> --- > drivers/gpu/drm/drm_fb_helper.c |4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 03414bd..dedf6e7 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper > *fb_helper, > if (modes[n] == NULL) > return best_score; > > + /* Writeback connectors aren't much use for fbdev */ > + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) > + return best_score; I think we could handle this by always marking writeback connectors as disconnected. Userspace and fbdev emulation should then avoid them, always. -Daniel > + > crtcs = kzalloc(fb_helper->connector_count * > sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); > if (!crtcs) > -- > 1.7.9.5 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch -- 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: [RFC PATCH 00/11] Introduce writeback connectors
On Tue, Oct 11, 2016 at 03:53:57PM +0100, Brian Starkey wrote: > Hi, > > This RFC series introduces a new connector type: > DRM_MODE_CONNECTOR_WRITEBACK > It is a follow-on from a previous discussion: [1] > > Writeback connectors are used to expose the memory writeback engines > found in some display controllers, which can write a CRTC's > composition result to a memory buffer. > This is useful e.g. for testing, screen-recording, screenshots, > wireless display, display cloning, memory-to-memory composition. > > Patches 1-7 include the core framework changes required, and patches > 8-11 implement a writeback connector for the Mali-DP writeback engine. > The Mali-DP patches depend on this other series: [2]. > > The connector is given the FB_ID property for the output framebuffer, > and two new read-only properties: PIXEL_FORMATS and > PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel > formats of the engine. > > The EDID property is not exposed for writeback connectors. > > Writeback connector usage: > -- > Due to connector routing changes being treated as "full modeset" > operations, any client which wishes to use a writeback connector > should include the connector in every modeset. The writeback will not > actually become active until a framebuffer is attached. Erhm, this is just the default, drivers can override this. And we could change the atomic helpers to not mark a modeset as a modeset if the connector that changed is a writeback one. > The writeback itself is enabled by attaching a framebuffer to the > FB_ID property of the connector. The driver must then ensure that the > CRTC content of that atomic commit is written into the framebuffer. > > The writeback works in a one-shot mode with each atomic commit. This > prevents the same content from being written multiple times. > In some cases (front-buffer rendering) there might be a desire for > continuous operation - I think a property could be added later for > this kind of control. > > Writeback can be disabled by setting FB_ID to zero. This seems to contradict itself: If it's one-shot, there's no need to disable it - it will auto-disable. In other cases where we write a property as a one-shot thing (fences for android). In that case when you read that property it's always 0 (well, -1 for fences since file descriptor). That also avoids the issues when userspace unconditionally saves/restores all properties (this is needed for generic compositor switching). I think a better behaviour would be to do the same trick, with FB_ID on the connector always returning 0 as the current value. That encodes the one-shot behaviour directly. For one-shot vs continuous: Maybe we want to simply have a separate writeback property for continues, e.g. FB_WRITEBACK_ONE_SHOT_ID and FB_WRITEBACK_CONTINUOUS_ID. > Known issues: > - > * I'm not sure what "DPMS" should mean for writeback connectors. >It could be used to disable writeback (even when a framebuffer is >attached), or it could be hidden entirely (which would break the >legacy DPMS call for writeback connectors). dpms is legacy, in atomic land the only thing you have is "ACTIVE" on the crtc. it disables everything, i.e. also writeback. > * With Daniel's recent re-iteration of the userspace API rules, I >fully expect to provide some userspace code to support this. The >question is what, and where? We want to use writeback for testing, >so perhaps some tests in igt is suitable. Hm, testing would be better as a debugfs interface, but I understand the appeal of doing this with atomic (since semantics fit so well). Another use-case of this is compositing, but if the main goal is igt and testing, I think integration into igt crc based testcases is a perfectly fine userspace. > * Documentation. Probably some portion of this cover letter needs to >make it into Documentation/ Yeah, an overview DOC: section in a separate source file (with all the the infrastructure work) would be great - aka needed from my pov ;-) > * Synchronisation. Our hardware will finish the writeback by the next >vsync. I've not implemented fence support here, but it would be an >obvious addition. Probably just want an additional WRITEBACK_FENCE_ID property to signal completion. Some hw definitely will take longer to write back than just a vblank. But we can delay that until it's needed. -Daniel > > See Also: > - > [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html > [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html > > I welcome any comments, especially if this approach does/doesn't fit > well with anyone else's hardware. > > Thanks, > > -Brian > > --- > > Brian Starkey (10): > drm: add writeback connector type > drm/fb-helper: skip writeback connectors > drm: extract CRTC/plane disable from drm_framebuffer_remove > drm: add __drm_framebuffer_remove_atomic >
Re: [PATCH 0/4] reST-directive kernel-cmd / include contentent from scripts
On Tue, 11 Oct 2016, Mauro Carvalho Chehabwrote: > Em Tue, 11 Oct 2016 09:26:48 +0200 > Markus Heiser escreveu: > >> Am 07.10.2016 um 07:56 schrieb Jani Nikula : >> >> > On Thu, 06 Oct 2016, Mauro Carvalho Chehab wrote: >> >> Em Thu, 06 Oct 2016 17:21:36 +0300 >> >> Jani Nikula escreveu: >> >>> We've seen what happens when we make it easy to add random scripts to >> >>> build documentation. We've worked hard to get rid of that. In my books, >> >>> one of the bigger points in favor of Sphinx over AsciiDoc(tor) was >> >>> getting rid of all the hacks required in the build. Things that broke in >> >>> subtle ways. >> >> >> >> I really can't see what scripts it get rids. >> > >> > Really? You don't see why the DocBook build was so fragile and difficult >> > to maintain? That scares me a bit, because then you will not have >> > learned why we should at all costs avoid adding random scripts to >> > produce documentation. >> >> For me, disassembling the DocBok build was hard and bothersome, I don't >> want this back. >> >> IMO: old hats are productive with perl and they won't adapt another >> interpreter language (like python) for scripting. >> >> This series -- the kernel-cmd -- directive avoid that they build >> fragile and difficult to maintain Makefile constructs, calling their >> perl scripts. >> >> Am 06.10.2016 um 16:21 schrieb Jani Nikula : >> >> > This is connected to the above: keeping documentation buildable with >> > sphinx-build directly will force you to avoid the Makefile hacks. >> >> >> Thats why I think, that the kernel-cmd directive is a more *straight- >> forward* solution, helps to **avoid** complexity while not everyone >> has to script in python ... >> >> > Case in point, parse-headers.pl was added for a specific need of media >> > documentation, and for the life of me I can't figure out by reading the >> > script what good, if any, it would be for gpu documentation. I call >> > *that* unmaintainable. >> >> >> If one adds a script like parse-headers.pl to the Documentation/sphinx >> folder, he/she also has to add a documentation to the >> kernel-documentation.rst >> >> If the kernel-cmd directive gets acked, I will add a description to >> kernel-documentation.rst and I request Mauro to document the parse-headers.pl >> also. > > I can write documentation for parse-headers.pl, either as a --help/--man > option or at some ReST file (or both). I'll add this to my mental TODO > list. Thanks, documentation will help everyone else evaluate whether parse-headers.pl is only useful for some corner case in media docs, or perhaps more generally useful. Currently, it's hard to tell. Anyway, documentation does not change my view on adding such scripts. As I've said, I think they will make the documentation build more difficult to maintain. They are likely to become special purpose hacks for corner cases across documentation. The rest of what you say is unrelated to the patches at hand. BR, Jani. > > - > > With regards to Sphinx x DocBook, in terms of functionality, Sphinx > is an improvement, as we can now use the same markup for everything, > and cross-reference all documents. Unfortunately, it has less > functionality than DocBook, and requires more magic to work. > > However, there are costs to pay on using a Python script like Sphinx. > I won't mention again the issues with Python itself and its unstable > API, but, instead, focus on Sphinx script itself. > > First of all, installing packages for Sphinx script to work is a lot > more complex, specially for the PDF output, as the LaTeX requirements are > very distribution specific, as some distributions package each LaTeX > extension as optional packages, and the required extensions used by > the Sphinx script are not documented. > > Also, the Sphinx script and its build logic is a lot more fragile than > the Makefile/xmlto that we use on DocBook, and this has nothing to do > with adding extra perl/python scripts. > > While it is clean for html output, it is very dirty when producing > a PDF output. > > It starts by generating its own Makefile for PDF builds, at the output > directory, with we have little control on it. > > Also, it seems that almost all books will need hacks to produce proper PDF, > as neither ReST or Sphinx extensions currently have proper support to adjust > things provided on DocBook, like setting page properties, enabling > image/table scaling, auto-numbering images/tables/examples or changing page > orientation in the middle of a document. > > The biggest issue I found is related to table outputs in PDF format, as > it does a very poor job adjusting table margins to fit inside the paper > margins. Also, the auto-generated TOC index on PDF doesn't match the > numeration of the HTML output. Currently, I didn't find any solution > for the latter
[RFC PATCH 04/11] drm: Add __drm_framebuffer_remove_atomic
Implement the CRTC/Plane disable functionality of drm_framebuffer_remove using the atomic API, and use it if possible. For atomic drivers, this removes the possibility of several commits when a framebuffer is in use by more than one CRTC/plane. Additionally, this will provide a suitable place to support the removal of a framebuffer from a writeback connector, in the case that a writeback connector is still actively using a framebuffer when it is removed by userspace. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_framebuffer.c | 154 - 1 file changed, 152 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 528f75d..b02cf73 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -22,6 +22,7 @@ #include #include +#include #include #include @@ -795,6 +796,148 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) EXPORT_SYMBOL(drm_framebuffer_cleanup); /** + * __drm_framebuffer_remove_atomic - atomic version of __drm_framebuffer_remove + * @dev: drm device + * @fb: framebuffer to remove + * + * If the driver implements the atomic API, we can handle the disabling of all + * CRTCs/planes which use a framebuffer which is going away in a single atomic + * commit. + * + * This scans all CRTCs and planes in @dev's mode_config. If they're using @fb, + * it is removed and the CRTC/plane disabled. + * The legacy references are dropped and the ->fb pointers set to NULL + * accordingly. + * + * Returns: + * true if the framebuffer was successfully removed from use + */ +static bool __drm_framebuffer_remove_atomic(struct drm_device *dev, + struct drm_framebuffer *fb) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + struct drm_connector_state *conn_state; + struct drm_connector *connector; + struct drm_plane *plane; + struct drm_crtc *crtc; + unsigned plane_mask; + int i, ret; + + drm_modeset_acquire_init(, 0); + + state = drm_atomic_state_alloc(dev); + if (!state) + return false; + + state->acquire_ctx = + +retry: + drm_for_each_crtc(crtc, dev) { + struct drm_plane_state *primary_state; + struct drm_crtc_state *crtc_state; + + primary_state = drm_atomic_get_plane_state(state, crtc->primary); + if (IS_ERR(primary_state)) { + ret = PTR_ERR(primary_state); + goto fail; + } + + if (primary_state->fb != fb) + continue; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) { + ret = PTR_ERR(crtc_state); + goto fail; + } + + /* Only handle the CRTC itself here, handle the plane later */ + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); + if (ret != 0) + goto fail; + + crtc_state->active = false; + + /* Get the connectors in order to disable them */ + ret = drm_atomic_add_affected_connectors(state, crtc); + if (ret) + goto fail; + } + + plane_mask = 0; + drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto fail; + } + + if (plane_state->fb != fb) + continue; + + plane->old_fb = plane->fb; + plane_mask |= 1 << drm_plane_index(plane); + + /* +* Open-coded copy of __drm_atomic_helper_disable_plane to avoid +* a dependency on atomic-helper +*/ + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret != 0) + goto fail; + + drm_atomic_set_fb_for_plane(plane_state, NULL); + plane_state->crtc_x = 0; + plane_state->crtc_y = 0; + plane_state->crtc_w = 0; + plane_state->crtc_h = 0; + plane_state->src_x = 0; + plane_state->src_y = 0; + plane_state->src_w = 0; + plane_state->src_h = 0; + } + + /* All of the connectors in state need disabling */ + for_each_connector_in_state(state, connector, conn_state, i) { + ret = drm_atomic_set_crtc_for_connector(conn_state, + NULL); + if (ret) + goto fail; + } + + if
[RFC PATCH 01/11] drm: Add writeback connector type
Writeback connectors represent writeback engines which can write the CRTC output to a memory framebuffer. Add a writeback connector type, hidden from userspace behind a client cap. They are hidden from non-aware clients so that they do not attempt to use writeback connectors to provide visual output to the user. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_connector.c |4 +++- drivers/gpu/drm/drm_crtc.c | 14 +- drivers/gpu/drm/drm_ioctl.c |7 +++ include/drm/drmP.h |2 ++ include/uapi/drm/drm.h | 10 ++ include/uapi/drm/drm_mode.h |1 + 6 files changed, 36 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 26bb78c7..027d7a9 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -86,6 +86,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = { { DRM_MODE_CONNECTOR_VIRTUAL, "Virtual" }, { DRM_MODE_CONNECTOR_DSI, "DSI" }, { DRM_MODE_CONNECTOR_DPI, "DPI" }, + { DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" }, }; void drm_connector_ida_init(void) @@ -235,7 +236,8 @@ int drm_connector_init(struct drm_device *dev, list_add_tail(>head, >connector_list); config->num_connector++; - if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL) + if ((connector_type != DRM_MODE_CONNECTOR_VIRTUAL) && + (connector_type != DRM_MODE_CONNECTOR_WRITEBACK)) drm_object_attach_property(>base, config->edid_property, 0); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2d7bedf..33f66e2 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -422,6 +422,14 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return 0; } +static bool +drm_connector_expose_to_userspace(const struct drm_connector *conn, + const struct drm_file *file_priv) +{ + return (file_priv->writeback_connectors) || + (conn->connector_type != DRM_MODE_CONNECTOR_WRITEBACK); +} + /** * drm_mode_getresources - get graphics configuration * @dev: drm device for the ioctl @@ -491,7 +499,8 @@ int drm_mode_getresources(struct drm_device *dev, void *data, crtc_count++; drm_for_each_connector(connector, dev) - connector_count++; + if (drm_connector_expose_to_userspace(connector, file_priv)) + connector_count++; drm_for_each_encoder(encoder, dev) encoder_count++; @@ -535,6 +544,9 @@ int drm_mode_getresources(struct drm_device *dev, void *data, copied = 0; connector_id = (uint32_t __user *)(unsigned long)card_res->connector_id_ptr; drm_for_each_connector(connector, dev) { + if (!drm_connector_expose_to_userspace(connector, file_priv)) + continue; + if (put_user(connector->base.id, connector_id + copied)) { ret = -EFAULT; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 0ad2c47..838a6e8 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -308,6 +308,13 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) file_priv->atomic = req->value; file_priv->universal_planes = req->value; break; + case DRM_CLIENT_CAP_WRITEBACK_CONNECTORS: + if (!drm_core_check_feature(dev, DRIVER_ATOMIC)) + return -EINVAL; + if (req->value > 1) + return -EINVAL; + file_priv->writeback_connectors = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 0e99669..222d5dc 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -388,6 +388,8 @@ struct drm_file { unsigned universal_planes:1; /* true if client understands atomic properties */ unsigned atomic:1; + /* true if client understands writeback connectors */ + unsigned writeback_connectors:1; /* * This client is the creator of @master. * Protected by struct drm_device::master_mutex. diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index b2c5284..d2b4543 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -678,6 +678,16 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_ATOMIC 3 +/** + * DRM_CLIENT_CAP_WRITEBACK_CONNECTORS + * + * If set to 1, the DRM core will expose writeback connectors to userspace. + * Writeback
[RFC PATCH 05/11] drm: Add fb to connector state
Add a framebuffer to the connector state, for use as the output target by writeback connectors. If a framebuffer is in use by a writeback connector when userspace removes it, it is handled by removing the framebuffer from the connector. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_atomic.c| 31 +++ drivers/gpu/drm/drm_atomic_helper.c |4 drivers/gpu/drm/drm_framebuffer.c | 24 include/drm/drm_atomic.h|3 +++ include/drm/drm_connector.h |3 +++ 5 files changed, 61 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 2373960..b16b4fc 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1205,6 +1205,37 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, EXPORT_SYMBOL(drm_atomic_set_crtc_for_connector); /** + * drm_atomic_set_fb_for_connector - set framebuffer for (writeback) connector + * @connector_state: atomic state object for the connector + * @fb: fb to use for the connector + * + * This is used to set the framebuffer for a writeback connector, which outputs + * to a buffer instead of an actual physical connector. + * Changing the assigned framebuffer requires us to grab a reference to the new + * fb and drop the reference to the old fb, if there is one. This function + * takes care of all these details besides updating the pointer in the + * state object itself. + */ +void +drm_atomic_set_fb_for_connector(struct drm_connector_state *conn_state, + struct drm_framebuffer *fb) +{ + if (conn_state->fb) + drm_framebuffer_unreference(conn_state->fb); + if (fb) + drm_framebuffer_reference(fb); + conn_state->fb = fb; + + if (fb) + DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n", +fb->base.id, conn_state); + else + DRM_DEBUG_ATOMIC("Set [NOFB] for connector state %p\n", +conn_state); +} +EXPORT_SYMBOL(drm_atomic_set_fb_for_connector); + +/** * drm_atomic_add_affected_connectors - add connectors for crtc * @state: atomic state * @crtc: DRM crtc diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3eecfc1..78ea735 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3234,6 +3234,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, memcpy(state, connector->state, sizeof(*state)); if (state->crtc) drm_connector_reference(connector); + if (state->fb) + drm_framebuffer_reference(state->fb); } EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); @@ -3361,6 +3363,8 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) */ if (state->crtc) drm_connector_unreference(state->connector); + if (state->fb) + drm_framebuffer_unreference(state->fb); } EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index b02cf73..f66908b1 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include "drm_crtc_internal.h" @@ -808,6 +809,8 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); * it is removed and the CRTC/plane disabled. * The legacy references are dropped and the ->fb pointers set to NULL * accordingly. + * It also checks for (writeback) connectors which are using @fb, and removes + * it if found. * * Returns: * true if the framebuffer was successfully removed from use @@ -900,7 +903,7 @@ retry: plane_state->src_h = 0; } - /* All of the connectors in state need disabling */ + /* All of the connectors currently in state need disabling */ for_each_connector_in_state(state, connector, conn_state, i) { ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); @@ -908,10 +911,23 @@ retry: goto fail; } - if (WARN_ON(!plane_mask)) { - DRM_ERROR("Couldn't find any usage of [FB:%d]\n", fb->base.id); - ret = -ENOENT; + /* Now find any writeback connectors that need handling */ + ret = drm_modeset_lock(>dev->mode_config.connection_mutex, + state->acquire_ctx); + if (ret) goto fail; + + drm_for_each_connector(connector, dev) { + conn_state = drm_atomic_get_connector_state(state, connector); + if (IS_ERR(conn_state)) { + ret =
[RFC PATCH 06/11] drm: Expose fb_id property for writeback connectors
Expose the framebuffer for writeback connectors to userspace by attaching the fb_id property to them. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_atomic.c|9 + drivers/gpu/drm/drm_connector.c |4 2 files changed, 13 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index b16b4fc..82e8e3a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -986,12 +986,19 @@ int drm_atomic_connector_set_property(struct drm_connector *connector, * now?) atomic writes to DPMS property: */ return -EINVAL; + } else if (property == config->prop_fb_id) { + struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val); + drm_atomic_set_fb_for_connector(state, fb); + if (fb) + drm_framebuffer_unreference(fb); } else if (connector->funcs->atomic_set_property) { return connector->funcs->atomic_set_property(connector, state, property, val); } else { return -EINVAL; } + + return 0; } EXPORT_SYMBOL(drm_atomic_connector_set_property); @@ -1022,6 +1029,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector, *val = (state->crtc) ? state->crtc->base.id : 0; } else if (property == config->dpms_property) { *val = connector->dpms; + } else if (property == config->prop_fb_id) { + *val = (state->fb) ? state->fb->base.id : 0; } else if (connector->funcs->atomic_get_property) { return connector->funcs->atomic_get_property(connector, state, property, val); diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 027d7a9..fb83870 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -249,6 +249,10 @@ int drm_connector_init(struct drm_device *dev, drm_object_attach_property(>base, config->prop_crtc_id, 0); } + if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) + drm_object_attach_property(>base, + config->prop_fb_id, 0); + connector->debugfs_entry = NULL; out_put_type_id: if (ret) -- 1.7.9.5 -- 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
[RFC PATCH 07/11] drm: Add writeback-connector pixel format properties
So that userspace can determine what pixel formats are supported for a writeback connector's framebuffer, add a pixel format list to writeback connectors. This is in the form of an immutable blob containing an array of formats, and an immutable uint holding the array size. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_connector.c | 73 ++- include/drm/drm_connector.h | 12 +++ include/drm/drm_crtc.h | 12 +++ 3 files changed, 96 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index fb83870..2f1f61d 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -249,9 +249,14 @@ int drm_connector_init(struct drm_device *dev, drm_object_attach_property(>base, config->prop_crtc_id, 0); } - if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) + if (connector_type == DRM_MODE_CONNECTOR_WRITEBACK) { drm_object_attach_property(>base, config->prop_fb_id, 0); + drm_object_attach_property(>base, + config->pixel_formats_property, 0); + drm_object_attach_property(>base, + config->pixel_formats_size_property, 0); + } connector->debugfs_entry = NULL; out_put_type_id: @@ -851,6 +856,45 @@ int drm_mode_create_suggested_offset_properties(struct drm_device *dev) EXPORT_SYMBOL(drm_mode_create_suggested_offset_properties); /** + * drm_mode_create_writeback_connector_properties - create writeback connector properties + * @dev: DRM device + * + * Create the properties specific to writeback connectors. These will be attached + * to writeback connectors by drm_connector_init. Drivers can set these + * properties using drm_mode_connector_set_writeback_formats(). + * + * "PIXEL_FORMATS": + * Immutable blob property to store the supported pixel formats table. The + * data is an array of u32 DRM_FORMAT_* fourcc values. + * Userspace can use this blob to find out what pixel formats are supported + * by the connector's writeback engine. + * + * "PIXEL_FORMATS_SIZE": + * Immutable unsigned range property storing the number of entries in the + * PIXEL_FORMATS array. + */ +int drm_mode_create_writeback_connector_properties(struct drm_device *dev) +{ + if (dev->mode_config.pixel_formats_property && + dev->mode_config.pixel_formats_size_property) + return 0; + + dev->mode_config.pixel_formats_property = + drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE, + "PIXEL_FORMATS", 0); + + dev->mode_config.pixel_formats_size_property = + drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE, + "PIXEL_FORMATS_SIZE", 0, UINT_MAX); + + if (dev->mode_config.pixel_formats_property == NULL || + dev->mode_config.pixel_formats_size_property == NULL) + return -ENOMEM; + return 0; +} +EXPORT_SYMBOL(drm_mode_create_writeback_connector_properties); + +/** * drm_mode_connector_set_path_property - set tile property on connector * @connector: connector to set property on. * @path: path to use for property; must not be NULL. @@ -957,6 +1001,33 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector, } EXPORT_SYMBOL(drm_mode_connector_update_edid_property); +int drm_mode_connector_set_writeback_formats(struct drm_connector *connector, +u32 *formats, +unsigned int n_formats) +{ + struct drm_device *dev = connector->dev; + size_t size = n_formats * sizeof(*formats); + int ret; + + if (connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK) + return -EINVAL; + + ret = drm_property_replace_global_blob(dev, + >pixel_formats_blob_ptr, + size, + formats, + >base, + dev->mode_config.pixel_formats_property); + + if (!ret) + drm_object_property_set_value(>base, + dev->mode_config.pixel_formats_size_property, + n_formats); + + return 0; +} +EXPORT_SYMBOL(drm_mode_connector_set_writeback_formats); + int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, struct drm_property *property, uint64_t value) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 30a766a..e77ae5c
[RFC PATCH 08/11] drm: mali-dp: Rename malidp_input_format
We're going to use the same format list for output formats, so rename everything related to input formats to avoid confusion. Signed-off-by: Brian StarkeyReviewed-by: Liviu Dudau --- drivers/gpu/drm/arm/malidp_hw.c | 24 drivers/gpu/drm/arm/malidp_hw.h |8 drivers/gpu/drm/arm/malidp_planes.c |8 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c index 7f4a0bd..44a9d10 100644 --- a/drivers/gpu/drm/arm/malidp_hw.c +++ b/drivers/gpu/drm/arm/malidp_hw.c @@ -21,7 +21,7 @@ #include "malidp_drv.h" #include "malidp_hw.h" -static const struct malidp_input_format malidp500_de_formats[] = { +static const struct malidp_format_id malidp500_de_formats[] = { /*fourcc, layers supporting the format, internal id */ { DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2, 0 }, { DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_GRAPHICS2, 1 }, @@ -69,7 +69,7 @@ static const struct malidp_input_format malidp500_de_formats[] = { { DRM_FORMAT_NV12, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 6) },\ { DRM_FORMAT_YUV420, DE_VIDEO1 | DE_VIDEO2, MALIDP_ID(5, 7) } -static const struct malidp_input_format malidp550_de_formats[] = { +static const struct malidp_format_id malidp550_de_formats[] = { MALIDP_COMMON_FORMATS, }; @@ -439,8 +439,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { .irq_mask = MALIDP500_DE_IRQ_CONF_VALID, .vsync_irq = MALIDP500_DE_IRQ_CONF_VALID, }, - .input_formats = malidp500_de_formats, - .n_input_formats = ARRAY_SIZE(malidp500_de_formats), + .pixel_formats = malidp500_de_formats, + .n_pixel_formats = ARRAY_SIZE(malidp500_de_formats), .bus_align_bytes = 8, }, .query_hw = malidp500_query_hw, @@ -472,8 +472,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { .irq_mask = MALIDP550_DC_IRQ_CONF_VALID, .vsync_irq = MALIDP550_DC_IRQ_CONF_VALID, }, - .input_formats = malidp550_de_formats, - .n_input_formats = ARRAY_SIZE(malidp550_de_formats), + .pixel_formats = malidp550_de_formats, + .n_pixel_formats = ARRAY_SIZE(malidp550_de_formats), .bus_align_bytes = 8, }, .query_hw = malidp550_query_hw, @@ -506,8 +506,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { .irq_mask = MALIDP550_DC_IRQ_CONF_VALID, .vsync_irq = MALIDP550_DC_IRQ_CONF_VALID, }, - .input_formats = malidp550_de_formats, - .n_input_formats = ARRAY_SIZE(malidp550_de_formats), + .pixel_formats = malidp550_de_formats, + .n_pixel_formats = ARRAY_SIZE(malidp550_de_formats), .bus_align_bytes = 16, }, .query_hw = malidp650_query_hw, @@ -525,10 +525,10 @@ u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map, { unsigned int i; - for (i = 0; i < map->n_input_formats; i++) { - if (((map->input_formats[i].layer & layer_id) == layer_id) && - (map->input_formats[i].format == format)) - return map->input_formats[i].id; + for (i = 0; i < map->n_pixel_formats; i++) { + if (((map->pixel_formats[i].layer & layer_id) == layer_id) && + (map->pixel_formats[i].format == format)) + return map->pixel_formats[i].id; } return MALIDP_INVALID_FORMAT_ID; diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h index 087e1202..4f8c884 100644 --- a/drivers/gpu/drm/arm/malidp_hw.h +++ b/drivers/gpu/drm/arm/malidp_hw.h @@ -35,7 +35,7 @@ enum { DE_SMART = BIT(4), }; -struct malidp_input_format { +struct malidp_format_id { u32 format; /* DRM fourcc */ u8 layer; /* bitmask of layers supporting it */ u8 id; /* used internally */ @@ -85,9 +85,9 @@ struct malidp_hw_regmap { const struct malidp_irq_map se_irq_map; const struct malidp_irq_map dc_irq_map; - /* list of supported input formats for each layer */ - const struct malidp_input_format *input_formats; - const u8 n_input_formats; + /* list of supported pixel formats for each layer */ + const struct malidp_format_id
[RFC PATCH 09/11] drm: mali-dp: Add RGB writeback formats for DP550/DP650
Add a layer bit for the SE memory-write, and add it to the pixel format matrix for DP550/DP650. Signed-off-by: Brian Starkey--- drivers/gpu/drm/arm/malidp_hw.c | 28 ++-- drivers/gpu/drm/arm/malidp_hw.h |1 + 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c index 44a9d10..5235d0b 100644 --- a/drivers/gpu/drm/arm/malidp_hw.c +++ b/drivers/gpu/drm/arm/malidp_hw.c @@ -46,20 +46,20 @@ static const struct malidp_format_id malidp500_de_formats[] = { #define MALIDP_COMMON_FORMATS \ /*fourcc, layers supporting the format, internal id */ \ - { DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 0) }, \ - { DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 1) }, \ - { DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 2) }, \ - { DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(0, 3) }, \ - { DRM_FORMAT_ARGB, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 0) }, \ - { DRM_FORMAT_ABGR, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 1) }, \ - { DRM_FORMAT_RGBA, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 2) }, \ - { DRM_FORMAT_BGRA, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(1, 3) }, \ - { DRM_FORMAT_XRGB, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 0) }, \ - { DRM_FORMAT_XBGR, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 1) }, \ - { DRM_FORMAT_RGBX, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 2) }, \ - { DRM_FORMAT_BGRX, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART, MALIDP_ID(2, 3) }, \ - { DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 0) }, \ - { DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(3, 1) }, \ + { DRM_FORMAT_ARGB2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 0) }, \ + { DRM_FORMAT_ABGR2101010, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 1) }, \ + { DRM_FORMAT_RGBA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 2) }, \ + { DRM_FORMAT_BGRA1010102, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(0, 3) }, \ + { DRM_FORMAT_ARGB, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 0) }, \ + { DRM_FORMAT_ABGR, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 1) }, \ + { DRM_FORMAT_RGBA, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 2) }, \ + { DRM_FORMAT_BGRA, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(1, 3) }, \ + { DRM_FORMAT_XRGB, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 0) }, \ + { DRM_FORMAT_XBGR, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 1) }, \ + { DRM_FORMAT_RGBX, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 2) }, \ + { DRM_FORMAT_BGRX, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | DE_SMART | SE_MEMWRITE, MALIDP_ID(2, 3) }, \ + { DRM_FORMAT_RGB888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 0) }, \ + { DRM_FORMAT_BGR888, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2 | SE_MEMWRITE, MALIDP_ID(3, 1) }, \ { DRM_FORMAT_RGBA5551, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 0) }, \ { DRM_FORMAT_ABGR1555, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 1) }, \ { DRM_FORMAT_RGB565, DE_VIDEO1 | DE_GRAPHICS1 | DE_VIDEO2, MALIDP_ID(4, 2) }, \ diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h index 4f8c884..ce4ea55 100644 --- a/drivers/gpu/drm/arm/malidp_hw.h +++ b/drivers/gpu/drm/arm/malidp_hw.h @@ -33,6 +33,7 @@ enum { DE_GRAPHICS2 = BIT(2), /* used only in DP500 */ DE_VIDEO2 = BIT(3), DE_SMART = BIT(4), + SE_MEMWRITE = BIT(5), }; struct malidp_format_id { -- 1.7.9.5 -- 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
[RFC PATCH 10/11] drm: mali-dp: Add support for writeback on DP550/DP650
From: Liviu DudauMali-DP display processors are able to write the composition result to a memory buffer via the SE. Add entry points in the HAL for enabling/disabling this feature, and implement support for it on DP650 and DP550. DP500 acts differently and so is omitted from this change. Signed-off-by: Liviu Dudau Signed-off-by: Brian Starkey --- drivers/gpu/drm/arm/malidp_hw.c | 52 +++-- drivers/gpu/drm/arm/malidp_hw.h | 18 + drivers/gpu/drm/arm/malidp_regs.h | 15 +++ 3 files changed, 83 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c index 5235d0b..dee7605 100644 --- a/drivers/gpu/drm/arm/malidp_hw.c +++ b/drivers/gpu/drm/arm/malidp_hw.c @@ -387,6 +387,48 @@ static int malidp550_rotmem_required(struct malidp_hw_device *hwdev, u16 w, u16 return w * bytes_per_col; } +static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev, +dma_addr_t *addrs, s32 *pitches, +int num_planes, u16 w, u16 h, u32 fmt_id) +{ + u32 base = MALIDP550_SE_MEMWRITE_BASE; + u32 de_base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK); + + /* enable the scaling engine block */ + malidp_hw_setbits(hwdev, MALIDP_SCALE_ENGINE_EN, de_base + MALIDP_DE_DISPLAY_FUNC); + + malidp_hw_write(hwdev, fmt_id, base + MALIDP_MW_FORMAT); + switch (num_planes) { + case 2: + malidp_hw_write(hwdev, lower_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_LOW); + malidp_hw_write(hwdev, upper_32_bits(addrs[1]), base + MALIDP_MW_P2_PTR_HIGH); + malidp_hw_write(hwdev, pitches[1], base + MALIDP_MW_P2_STRIDE); + /* fall through */ + case 1: + malidp_hw_write(hwdev, lower_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_LOW); + malidp_hw_write(hwdev, upper_32_bits(addrs[0]), base + MALIDP_MW_P1_PTR_HIGH); + malidp_hw_write(hwdev, pitches[0], base + MALIDP_MW_P1_STRIDE); + break; + default: + WARN(1, "Invalid number of planes"); + } + + malidp_hw_write(hwdev, MALIDP_DE_H_ACTIVE(w) | MALIDP_DE_V_ACTIVE(h), + MALIDP550_SE_MEMWRITE_OUT_SIZE); + malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN, + MALIDP550_SE_CONTROL); + + return 0; +} + +static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev) +{ + u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK); + malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN, + MALIDP550_SE_CONTROL); + malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC); +} + static int malidp650_query_hw(struct malidp_hw_device *hwdev) { u32 conf = malidp_hw_read(hwdev, MALIDP550_CONFIG_ID); @@ -469,7 +511,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { MALIDP550_SE_IRQ_AXI_ERR, }, .dc_irq_map = { - .irq_mask = MALIDP550_DC_IRQ_CONF_VALID, + .irq_mask = MALIDP550_DC_IRQ_CONF_VALID | + MALIDP550_DC_IRQ_SE, .vsync_irq = MALIDP550_DC_IRQ_CONF_VALID, }, .pixel_formats = malidp550_de_formats, @@ -483,6 +526,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { .set_config_valid = malidp550_set_config_valid, .modeset = malidp550_modeset, .rotmem_required = malidp550_rotmem_required, + .enable_memwrite = malidp550_enable_memwrite, + .disable_memwrite = malidp550_disable_memwrite, }, [MALIDP_650] = { .map = { @@ -503,7 +548,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { MALIDP550_SE_IRQ_AXI_ERR, }, .dc_irq_map = { - .irq_mask = MALIDP550_DC_IRQ_CONF_VALID, + .irq_mask = MALIDP550_DC_IRQ_CONF_VALID | + MALIDP550_DC_IRQ_SE, .vsync_irq = MALIDP550_DC_IRQ_CONF_VALID, }, .pixel_formats = malidp550_de_formats, @@ -517,6 +563,8 @@ const struct malidp_hw_device malidp_device[MALIDP_MAX_DEVICES] = { .set_config_valid = malidp550_set_config_valid, .modeset = malidp550_modeset, .rotmem_required = malidp550_rotmem_required,
[RFC PATCH 11/11] drm: mali-dp: Add writeback connector
Mali-DP has a memory writeback engine which can be used to write the composition result to a memory buffer. Expose this functionality as a DRM writeback connector on supported hardware. Signed-off-by: Brian Starkey--- drivers/gpu/drm/arm/Makefile |1 + drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ drivers/gpu/drm/arm/malidp_drv.c | 25 +++- drivers/gpu/drm/arm/malidp_drv.h |5 + drivers/gpu/drm/arm/malidp_mw.c | 268 + 5 files changed, 305 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/arm/malidp_mw.c diff --git a/drivers/gpu/drm/arm/Makefile b/drivers/gpu/drm/arm/Makefile index bb8b158..3bf31d1 100644 --- a/drivers/gpu/drm/arm/Makefile +++ b/drivers/gpu/drm/arm/Makefile @@ -1,4 +1,5 @@ hdlcd-y := hdlcd_drv.o hdlcd_crtc.o obj-$(CONFIG_DRM_HDLCD)+= hdlcd.o mali-dp-y := malidp_drv.o malidp_hw.o malidp_planes.o malidp_crtc.o +mali-dp-y += malidp_mw.o obj-$(CONFIG_DRM_MALI_DISPLAY) += mali-dp.o diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 08e6a71..98ddcea 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -68,6 +68,16 @@ static void malidp_crtc_enable(struct drm_crtc *crtc) clk_set_rate(hwdev->pxlclk, crtc->state->adjusted_mode.crtc_clock * 1000); hwdev->modeset(hwdev, ); + /* +* We should always disable the memory write when leaving config mode, +* otherwise the hardware will start writing right away - possibly with +* a stale config, and definitely before we've had a chance to configure +* the planes. +* If the memory write needs to be enabled, that will get taken care +* of later during the atomic commit +*/ + if (hwdev->disable_memwrite) + hwdev->disable_memwrite(hwdev); hwdev->leave_config_mode(hwdev); drm_crtc_vblank_on(crtc); } diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 62a29f6..e20266e 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -91,7 +91,16 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state) struct drm_device *drm = state->dev; drm_atomic_helper_commit_modeset_disables(drm, state); + drm_atomic_helper_commit_modeset_enables(drm, state); + + /* +* The order here is important. We must configure memory-write after +* the CRTC is already enabled, so that its configuration update is +* gated on the next CVAL. +*/ + malidp_mw_atomic_commit(drm, state); + drm_atomic_helper_commit_planes(drm, state, DRM_PLANE_COMMIT_ACTIVE_ONLY); @@ -148,12 +157,20 @@ static int malidp_init(struct drm_device *drm) drm->mode_config.helper_private = _mode_config_helpers; ret = malidp_crtc_init(drm); - if (ret) { - drm_mode_config_cleanup(drm); - return ret; - } + if (ret) + goto crtc_fail; + + ret = malidp_mw_connector_init(drm); + if (ret) + goto mw_fail; return 0; + +mw_fail: + malidp_de_planes_destroy(drm); +crtc_fail: + drm_mode_config_cleanup(drm); + return ret; } static void malidp_fini(struct drm_device *drm) diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h index 9fc8a2e..905c104 100644 --- a/drivers/gpu/drm/arm/malidp_drv.h +++ b/drivers/gpu/drm/arm/malidp_drv.h @@ -22,6 +22,8 @@ struct malidp_drm { struct drm_fbdev_cma *fbdev; struct list_head event_list; struct drm_crtc crtc; + struct drm_encoder mw_encoder; + struct drm_connector mw_connector; wait_queue_head_t wq; atomic_t config_valid; }; @@ -50,6 +52,9 @@ struct malidp_plane_state { int malidp_de_planes_init(struct drm_device *drm); void malidp_de_planes_destroy(struct drm_device *drm); int malidp_crtc_init(struct drm_device *drm); +int malidp_mw_connector_init(struct drm_device *drm); +void malidp_mw_atomic_commit(struct drm_device *drm, +struct drm_atomic_state *old_state); /* often used combination of rotational bits */ #define MALIDP_ROTATED_MASK(DRM_ROTATE_90 | DRM_ROTATE_270) diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c new file mode 100644 index 000..72df3fd --- /dev/null +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -0,0 +1,268 @@ +/* + * (C) COPYRIGHT 2016 ARM Limited. All rights reserved. + * Author: Brian Starkey + * + * This program is free software and is provided to you under the terms of the + * GNU General Public License version 2 as published by the Free Software + * Foundation, and any use by you of this program is subject to the terms + * of such GNU licence. + * + * ARM Mali DP Writeback connector
[RFC PATCH 00/11] Introduce writeback connectors
Hi, This RFC series introduces a new connector type: DRM_MODE_CONNECTOR_WRITEBACK It is a follow-on from a previous discussion: [1] Writeback connectors are used to expose the memory writeback engines found in some display controllers, which can write a CRTC's composition result to a memory buffer. This is useful e.g. for testing, screen-recording, screenshots, wireless display, display cloning, memory-to-memory composition. Patches 1-7 include the core framework changes required, and patches 8-11 implement a writeback connector for the Mali-DP writeback engine. The Mali-DP patches depend on this other series: [2]. The connector is given the FB_ID property for the output framebuffer, and two new read-only properties: PIXEL_FORMATS and PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel formats of the engine. The EDID property is not exposed for writeback connectors. Writeback connector usage: -- Due to connector routing changes being treated as "full modeset" operations, any client which wishes to use a writeback connector should include the connector in every modeset. The writeback will not actually become active until a framebuffer is attached. The writeback itself is enabled by attaching a framebuffer to the FB_ID property of the connector. The driver must then ensure that the CRTC content of that atomic commit is written into the framebuffer. The writeback works in a one-shot mode with each atomic commit. This prevents the same content from being written multiple times. In some cases (front-buffer rendering) there might be a desire for continuous operation - I think a property could be added later for this kind of control. Writeback can be disabled by setting FB_ID to zero. Known issues: - * I'm not sure what "DPMS" should mean for writeback connectors. It could be used to disable writeback (even when a framebuffer is attached), or it could be hidden entirely (which would break the legacy DPMS call for writeback connectors). * With Daniel's recent re-iteration of the userspace API rules, I fully expect to provide some userspace code to support this. The question is what, and where? We want to use writeback for testing, so perhaps some tests in igt is suitable. * Documentation. Probably some portion of this cover letter needs to make it into Documentation/ * Synchronisation. Our hardware will finish the writeback by the next vsync. I've not implemented fence support here, but it would be an obvious addition. See Also: - [1] https://lists.freedesktop.org/archives/dri-devel/2016-July/113197.html [2] https://lists.freedesktop.org/archives/dri-devel/2016-October/120486.html I welcome any comments, especially if this approach does/doesn't fit well with anyone else's hardware. Thanks, -Brian --- Brian Starkey (10): drm: add writeback connector type drm/fb-helper: skip writeback connectors drm: extract CRTC/plane disable from drm_framebuffer_remove drm: add __drm_framebuffer_remove_atomic drm: add fb to connector state drm: expose fb_id property for writeback connectors drm: add writeback-connector pixel format properties drm: mali-dp: rename malidp_input_format drm: mali-dp: add RGB writeback formats for DP550/DP650 drm: mali-dp: add writeback connector Liviu Dudau (1): drm: mali-dp: Add support for writeback on DP550/DP650 drivers/gpu/drm/arm/Makefile|1 + drivers/gpu/drm/arm/malidp_crtc.c | 10 ++ drivers/gpu/drm/arm/malidp_drv.c| 25 +++- drivers/gpu/drm/arm/malidp_drv.h|5 + drivers/gpu/drm/arm/malidp_hw.c | 104 ++ drivers/gpu/drm/arm/malidp_hw.h | 27 +++- drivers/gpu/drm/arm/malidp_mw.c | 268 +++ drivers/gpu/drm/arm/malidp_planes.c |8 +- drivers/gpu/drm/arm/malidp_regs.h | 15 ++ drivers/gpu/drm/drm_atomic.c| 40 ++ drivers/gpu/drm/drm_atomic_helper.c |4 + drivers/gpu/drm/drm_connector.c | 79 ++- drivers/gpu/drm/drm_crtc.c | 14 +- drivers/gpu/drm/drm_fb_helper.c |4 + drivers/gpu/drm/drm_framebuffer.c | 249 drivers/gpu/drm/drm_ioctl.c |7 + include/drm/drmP.h |2 + include/drm/drm_atomic.h|3 + include/drm/drm_connector.h | 15 ++ include/drm/drm_crtc.h | 12 ++ include/uapi/drm/drm.h | 10 ++ include/uapi/drm/drm_mode.h |1 + 22 files changed, 830 insertions(+), 73 deletions(-) create mode 100644 drivers/gpu/drm/arm/malidp_mw.c -- 1.7.9.5 -- 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
[RFC PATCH 03/11] drm: Extract CRTC/plane disable from drm_framebuffer_remove
In preparation for adding an atomic version of the disable code, extract the actual disable operation into a separate function. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_framebuffer.c | 87 +++-- 1 file changed, 54 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 398efd6..528f75d 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -795,22 +795,61 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb) EXPORT_SYMBOL(drm_framebuffer_cleanup); /** - * drm_framebuffer_remove - remove and unreference a framebuffer object + * __drm_framebuffer_remove - remove all usage of a framebuffer object + * @dev: drm device * @fb: framebuffer to remove * * Scans all the CRTCs and planes in @dev's mode_config. If they're - * using @fb, removes it, setting it to NULL. Then drops the reference to the - * passed-in framebuffer. Might take the modeset locks. + * using @fb, removes it, setting it to NULL. Takes the modeset locks. * - * Note that this function optimizes the cleanup away if the caller holds the - * last reference to the framebuffer. It is also guaranteed to not take the - * modeset locks in this case. + * Returns: + * true if the framebuffer was successfully removed from use */ -void drm_framebuffer_remove(struct drm_framebuffer *fb) +static bool __drm_framebuffer_remove(struct drm_device *dev, struct drm_framebuffer *fb) { - struct drm_device *dev; struct drm_crtc *crtc; struct drm_plane *plane; + bool ret = true; + + drm_modeset_lock_all(dev); + /* remove from any CRTC */ + drm_for_each_crtc(crtc, dev) { + if (crtc->primary->fb == fb) { + /* should turn off the crtc */ + if (drm_crtc_force_disable(crtc)) + ret = false; + } + } + + drm_for_each_plane(plane, dev) { + if (plane->fb == fb) + /* TODO: Propagate error here? */ + drm_plane_force_disable(plane); + } + drm_modeset_unlock_all(dev); + + return ret; +} + +/** + * drm_framebuffer_remove - remove and unreference a framebuffer object + * @fb: framebuffer to remove + * + * drm ABI mandates that we remove any deleted framebuffers from active usage. + * This function takes care of this detail, disabling any CRTCs/Planes which + * are using the framebuffer being removed. + * + * Since most sane clients only remove framebuffers they no longer need, we + * skip the disable step if the caller holds the last reference to the + * framebuffer. It is also guaranteed to not take the modeset locks in + * this case. + * + * Before returning this function drops (what should be) the last reference + * on the framebuffer. + */ +void drm_framebuffer_remove(struct drm_framebuffer *fb) +{ + struct drm_device *dev; if (!fb) return; @@ -820,37 +859,19 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) WARN_ON(!list_empty(>filp_head)); /* -* drm ABI mandates that we remove any deleted framebuffers from active -* useage. But since most sane clients only remove framebuffers they no -* longer need, try to optimize this away. -* * Since we're holding a reference ourselves, observing a refcount of 1 -* means that we're the last holder and can skip it. Also, the refcount -* can never increase from 1 again, so we don't need any barriers or -* locks. +* means that we're the last holder and can skip the disable. Also, the +* refcount can never increase from 1 again, so we don't need any +* barriers or locks. * -* Note that userspace could try to race with use and instate a new +* Note that userspace could try to race with us and instate a new * usage _after_ we've cleared all current ones. End result will be an * in-use fb with fb-id == 0. Userspace is allowed to shoot its own foot * in this manner. */ - if (drm_framebuffer_read_refcount(fb) > 1) { - drm_modeset_lock_all(dev); - /* remove from any CRTC */ - drm_for_each_crtc(crtc, dev) { - if (crtc->primary->fb == fb) { - /* should turn off the crtc */ - if (drm_crtc_force_disable(crtc)) - DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); - } - } - - drm_for_each_plane(plane, dev) { - if (plane->fb == fb) - drm_plane_force_disable(plane); - } - drm_modeset_unlock_all(dev); - } + if
[RFC PATCH 02/11] drm/fb-helper: Skip writeback connectors
Writeback connectors aren't much use to the fbdev helpers, as they won't show anything to the user. Skip them when looking for candidate output configurations. Signed-off-by: Brian Starkey--- drivers/gpu/drm/drm_fb_helper.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 03414bd..dedf6e7 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2016,6 +2016,10 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper, if (modes[n] == NULL) return best_score; + /* Writeback connectors aren't much use for fbdev */ + if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK) + return best_score; + crtcs = kzalloc(fb_helper->connector_count * sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); if (!crtcs) -- 1.7.9.5 -- 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 0/4] reST-directive kernel-cmd / include contentent from scripts
Em Tue, 11 Oct 2016 09:26:48 +0200 Markus Heiserescreveu: > Am 07.10.2016 um 07:56 schrieb Jani Nikula : > > > On Thu, 06 Oct 2016, Mauro Carvalho Chehab wrote: > >> Em Thu, 06 Oct 2016 17:21:36 +0300 > >> Jani Nikula escreveu: > >>> We've seen what happens when we make it easy to add random scripts to > >>> build documentation. We've worked hard to get rid of that. In my books, > >>> one of the bigger points in favor of Sphinx over AsciiDoc(tor) was > >>> getting rid of all the hacks required in the build. Things that broke in > >>> subtle ways. > >> > >> I really can't see what scripts it get rids. > > > > Really? You don't see why the DocBook build was so fragile and difficult > > to maintain? That scares me a bit, because then you will not have > > learned why we should at all costs avoid adding random scripts to > > produce documentation. > > For me, disassembling the DocBok build was hard and bothersome, I don't > want this back. > > IMO: old hats are productive with perl and they won't adapt another > interpreter language (like python) for scripting. > > This series -- the kernel-cmd -- directive avoid that they build > fragile and difficult to maintain Makefile constructs, calling their > perl scripts. > > Am 06.10.2016 um 16:21 schrieb Jani Nikula : > > > This is connected to the above: keeping documentation buildable with > > sphinx-build directly will force you to avoid the Makefile hacks. > > > Thats why I think, that the kernel-cmd directive is a more *straight- > forward* solution, helps to **avoid** complexity while not everyone > has to script in python ... > > > Case in point, parse-headers.pl was added for a specific need of media > > documentation, and for the life of me I can't figure out by reading the > > script what good, if any, it would be for gpu documentation. I call > > *that* unmaintainable. > > > If one adds a script like parse-headers.pl to the Documentation/sphinx > folder, he/she also has to add a documentation to the kernel-documentation.rst > > If the kernel-cmd directive gets acked, I will add a description to > kernel-documentation.rst and I request Mauro to document the parse-headers.pl > also. I can write documentation for parse-headers.pl, either as a --help/--man option or at some ReST file (or both). I'll add this to my mental TODO list. - With regards to Sphinx x DocBook, in terms of functionality, Sphinx is an improvement, as we can now use the same markup for everything, and cross-reference all documents. Unfortunately, it has less functionality than DocBook, and requires more magic to work. However, there are costs to pay on using a Python script like Sphinx. I won't mention again the issues with Python itself and its unstable API, but, instead, focus on Sphinx script itself. First of all, installing packages for Sphinx script to work is a lot more complex, specially for the PDF output, as the LaTeX requirements are very distribution specific, as some distributions package each LaTeX extension as optional packages, and the required extensions used by the Sphinx script are not documented. Also, the Sphinx script and its build logic is a lot more fragile than the Makefile/xmlto that we use on DocBook, and this has nothing to do with adding extra perl/python scripts. While it is clean for html output, it is very dirty when producing a PDF output. It starts by generating its own Makefile for PDF builds, at the output directory, with we have little control on it. Also, it seems that almost all books will need hacks to produce proper PDF, as neither ReST or Sphinx extensions currently have proper support to adjust things provided on DocBook, like setting page properties, enabling image/table scaling, auto-numbering images/tables/examples or changing page orientation in the middle of a document. The biggest issue I found is related to table outputs in PDF format, as it does a very poor job adjusting table margins to fit inside the paper margins. Also, the auto-generated TOC index on PDF doesn't match the numeration of the HTML output. Currently, I didn't find any solution for the latter issue. As Sphinx markup doesn't have any way to tell how the output would be formatted, a lot of magic is required at Sphinx conf.py for PDF output to start working. Worse than that, several tables require extra tags for PDF output, specifying the column sizes, and forcing them to be handled as longtables[1]. Also, if the table is too big, the rst files require raw latex macros/code before/after such tables, in order for them to fit inside a paper page - either by changing their orientation or enabling auto-scale, if the table is not a longtable. Currently, no way to tell Sphinx to enable auto-scale on big tables. So, in practice, it means that Sphinx build is a house of cards, at least for PDF output, as every new
[PATCH] pctv452e: fix semicolon.cocci warnings
drivers/media/usb/dvb-usb/pctv452e.c:115:2-3: Unneeded semicolon Remove unneeded semicolon. Generated by: scripts/coccinelle/misc/semicolon.cocci CC: Mauro Carvalho ChehabSigned-off-by: Fengguang Wu --- pctv452e.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/media/usb/dvb-usb/pctv452e.c +++ b/drivers/media/usb/dvb-usb/pctv452e.c @@ -112,7 +112,7 @@ static int tt3650_ci_msg(struct dvb_usb_ if (!data || (write_len > 64 - 4) || (read_len > 64 - 4)) { err("%s: transfer data invalid", __func__); return -EIO; - }; + } mutex_lock(>ca_mutex); id = state->c++; -- 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
[linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 22/31] drivers/media/usb/dvb-usb/pctv452e.c:115:2-3: Unneeded semicolon
tree: https://github.com/0day-ci/linux Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 head: ff49f775552fe4ebe2944527cf882073679cb1e5 commit: 4bafb476079bf7e4aa258248cfa853f130c46c8c [22/31] pctv452e: don't call BUG_ON() on non-fatal error coccinelle warnings: (new ones prefixed by >>) >> drivers/media/usb/dvb-usb/pctv452e.c:115:2-3: Unneeded semicolon Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- 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: [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169
On Tue, 11 Oct 2016, Julia Lawall wrote: > It looks like a lock may be needed before line 174. Sorry, an unlock. > > julia > > -- Forwarded message -- > Date: Tue, 11 Oct 2016 21:06:18 +0800 > From: kbuild test robot <fengguang...@intel.com> > To: kbu...@01.org > Cc: Julia Lawall <julia.law...@lip6.fr> > Subject: > > [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-medi > a-usb-drivers/20161011-182408 3/31] > drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line > 169 > > CC: kbuild-...@01.org > TO: Mauro Carvalho Chehab <m.che...@samsung.com> > CC: linux-media@vger.kernel.org > CC: 0day robot <fengguang...@intel.com> > > tree: https://github.com/0day-ci/linux > Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 > head: ff49f775552fe4ebe2944527cf882073679cb1e5 > commit: b38d98275e144aaea9db69ba2dcba58466046d9b [3/31] cinergyT2-core: > handle error code on RC query > :: branch date: 3 hours ago > :: commit date: 3 hours ago > > >> drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line > >> 169 > > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout b38d98275e144aaea9db69ba2dcba58466046d9b > vim +174 drivers/media/usb/dvb-usb/cinergyT2-core.c > > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > 2008-09-19 163 { > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > 2008-09-19 164 struct cinergyt2_state *st = d->priv; > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 165 int i, ret; > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > 2008-09-19 166 > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > 2008-09-19 167 *state = REMOTE_NO_KEY_PRESSED; > 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava > 2008-09-19 168 > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 @169 mutex_lock(>data_mutex); > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 170 st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 171 > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 172 ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 173 if (ret < 0) > b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 @174 return ret; > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 175 > 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab > 2016-10-11 176 if (st->data[4] == 0xff) { > 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE > 2008-09-19 177 /* key repeat */ > > --- > 0-DAY kernel test infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation > -- 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
[linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169
It looks like a lock may be needed before line 174. julia -- Forwarded message -- Date: Tue, 11 Oct 2016 21:06:18 +0800 From: kbuild test robot <fengguang...@intel.com> To: kbu...@01.org Cc: Julia Lawall <julia.law...@lip6.fr> Subject: [linux-review:Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-medi a-usb-drivers/20161011-182408 3/31] drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line 169 CC: kbuild-...@01.org TO: Mauro Carvalho Chehab <m.che...@samsung.com> CC: linux-media@vger.kernel.org CC: 0day robot <fengguang...@intel.com> tree: https://github.com/0day-ci/linux Mauro-Carvalho-Chehab/Don-t-use-stack-for-DMA-transers-on-media-usb-drivers/20161011-182408 head: ff49f775552fe4ebe2944527cf882073679cb1e5 commit: b38d98275e144aaea9db69ba2dcba58466046d9b [3/31] cinergyT2-core: handle error code on RC query :: branch date: 3 hours ago :: commit date: 3 hours ago >> drivers/media/usb/dvb-usb/cinergyT2-core.c:174:2-8: preceding lock on line >> 169 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout b38d98275e144aaea9db69ba2dcba58466046d9b vim +174 drivers/media/usb/dvb-usb/cinergyT2-core.c 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava 2008-09-19 163 { 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE 2008-09-19 164 struct cinergyt2_state *st = d->priv; b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 165 int i, ret; 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE 2008-09-19 166 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava 2008-09-19 167 *state = REMOTE_NO_KEY_PRESSED; 986bd1e5 drivers/media/dvb/dvb-usb/cinergyT2-core.c Tomi Orava 2008-09-19 168 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 @169 mutex_lock(>data_mutex); 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 170 st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 171 b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 172 ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 173 if (ret < 0) b38d9827 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 @174 return ret; 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 175 48922468 drivers/media/usb/dvb-usb/cinergyT2-core.c Mauro Carvalho Chehab 2016-10-11 176 if (st->data[4] == 0xff) { 7f987678 drivers/media/dvb/dvb-usb/cinergyT2-core.c Thierry MERLE 2008-09-19 177 /* key repeat */ --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- 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 v2 22/31] pctv452e: don't call BUG_ON() on non-fatal error
There are some conditions on this driver that are tested with BUG_ON() with are not serious enough to hang a machine. So, just return an error if this happens. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/pctv452e.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c index 58b685094904..7ad0006c5ae0 100644 --- a/drivers/media/usb/dvb-usb/pctv452e.c +++ b/drivers/media/usb/dvb-usb/pctv452e.c @@ -109,9 +109,10 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, u8 *data, unsigned int rlen; int ret; - BUG_ON(NULL == data && 0 != (write_len | read_len)); - BUG_ON(write_len > 64 - 4); - BUG_ON(read_len > 64 - 4); + if (!data || (write_len > 64 - 4) || (read_len > 64 - 4)) { + err("%s: transfer data invalid", __func__); + return -EIO; + }; mutex_lock(>ca_mutex); id = state->c++; -- 2.7.4 -- 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 v2 24/31] dvb-usb: warn if return value for USB read/write routines is not checked
the return values for dvb_usb_generic_rw() and dvb_usb_generic_write() should be checked, as otherwise the drivers won't be doing the right thing in the case of errors. So, add __must_check to both declarations. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dvb-usb.h | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dvb-usb.h b/drivers/media/usb/dvb-usb/dvb-usb.h index 639c4678c65b..1448c3d27ea2 100644 --- a/drivers/media/usb/dvb-usb/dvb-usb.h +++ b/drivers/media/usb/dvb-usb/dvb-usb.h @@ -462,8 +462,10 @@ extern int dvb_usb_device_init(struct usb_interface *, extern void dvb_usb_device_exit(struct usb_interface *); /* the generic read/write method for device control */ -extern int dvb_usb_generic_rw(struct dvb_usb_device *, u8 *, u16, u8 *, u16,int); -extern int dvb_usb_generic_write(struct dvb_usb_device *, u8 *, u16); +extern int __must_check +dvb_usb_generic_rw(struct dvb_usb_device *, u8 *, u16, u8 *, u16, int); +extern int __must_check +dvb_usb_generic_write(struct dvb_usb_device *, u8 *, u16); /* commonly used remote control parsing */ extern int dvb_usb_nec_rc_key_to_event(struct dvb_usb_device *, u8[], u32 *, int *); -- 2.7.4 -- 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 v2 07/31] dib0700: be sure that dib0700_ctrl_rd() users can do DMA
dib0700_ctrl_rd() takes a RX and a TX pointer. Be sure that both will point to a memory allocated via kmalloc(). Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/dib0700_core.c| 4 +++- drivers/media/usb/dvb-usb/dib0700_devices.c | 25 + 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c index f3196658fb70..515f89dba199 100644 --- a/drivers/media/usb/dvb-usb/dib0700_core.c +++ b/drivers/media/usb/dvb-usb/dib0700_core.c @@ -292,13 +292,15 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap, /* special thing in the current firmware: when length is zero the read-failed */ len = dib0700_ctrl_rd(d, st->buf, msg[i].len + 2, - msg[i+1].buf, msg[i+1].len); + st->buf, msg[i + 1].len); if (len <= 0) { deb_info("I2C read failed on address 0x%02x\n", msg[i].addr); break; } + memcpy(msg[i + 1].buf, st->buf, msg[i + 1].len); + msg[i+1].len = len; i++; diff --git a/drivers/media/usb/dvb-usb/dib0700_devices.c b/drivers/media/usb/dvb-usb/dib0700_devices.c index 0857b56e652c..ef1b8ee75c57 100644 --- a/drivers/media/usb/dvb-usb/dib0700_devices.c +++ b/drivers/media/usb/dvb-usb/dib0700_devices.c @@ -508,8 +508,6 @@ static int stk7700ph_tuner_attach(struct dvb_usb_adapter *adap) #define DEFAULT_RC_INTERVAL 50 -static u8 rc_request[] = { REQUEST_POLL_RC, 0 }; - /* * This function is used only when firmware is < 1.20 version. Newer * firmwares use bulk mode, with functions implemented at dib0700_core, @@ -517,7 +515,6 @@ static u8 rc_request[] = { REQUEST_POLL_RC, 0 }; */ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) { - u8 key[4]; enum rc_type protocol; u32 scancode; u8 toggle; @@ -532,39 +529,43 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) return 0; } - i = dib0700_ctrl_rd(d, rc_request, 2, key, 4); + st->buf[0] = REQUEST_POLL_RC; + st->buf[1] = 0; + + i = dib0700_ctrl_rd(d, st->buf, 2, st->buf, 4); if (i <= 0) { err("RC Query Failed"); - return -1; + return -EIO; } /* losing half of KEY_0 events from Philipps rc5 remotes.. */ - if (key[0] == 0 && key[1] == 0 && key[2] == 0 && key[3] == 0) + if (st->buf[0] == 0 && st->buf[1] == 0 + && st->buf[2] == 0 && st->buf[3] == 0) return 0; - /* info("%d: %2X %2X %2X %2X",dvb_usb_dib0700_ir_proto,(int)key[3-2],(int)key[3-3],(int)key[3-1],(int)key[3]); */ + /* info("%d: %2X %2X %2X %2X",dvb_usb_dib0700_ir_proto,(int)st->buf[3 - 2],(int)st->buf[3 - 3],(int)st->buf[3 - 1],(int)st->buf[3]); */ dib0700_rc_setup(d, NULL); /* reset ir sensor data to prevent false events */ switch (d->props.rc.core.protocol) { case RC_BIT_NEC: /* NEC protocol sends repeat code as 0 0 0 FF */ - if ((key[3-2] == 0x00) && (key[3-3] == 0x00) && - (key[3] == 0xff)) { + if ((st->buf[3 - 2] == 0x00) && (st->buf[3 - 3] == 0x00) && + (st->buf[3] == 0xff)) { rc_repeat(d->rc_dev); return 0; } protocol = RC_TYPE_NEC; - scancode = RC_SCANCODE_NEC(key[3-2], key[3-3]); + scancode = RC_SCANCODE_NEC(st->buf[3 - 2], st->buf[3 - 3]); toggle = 0; break; default: /* RC-5 protocol changes toggle bit on new keypress */ protocol = RC_TYPE_RC5; - scancode = RC_SCANCODE_RC5(key[3-2], key[3-3]); - toggle = key[3-1]; + scancode = RC_SCANCODE_RC5(st->buf[3 - 2], st->buf[3 - 3]); + toggle = st->buf[3 - 1]; break; } -- 2.7.4 -- 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 v2 30/31] stk-webcam: don't use stack for DMA
From: Mauro Carvalho ChehabThe USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/usb/stkwebcam/stk-webcam.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c index db200c9d796d..22a9aae16291 100644 --- a/drivers/media/usb/stkwebcam/stk-webcam.c +++ b/drivers/media/usb/stkwebcam/stk-webcam.c @@ -147,20 +147,26 @@ int stk_camera_write_reg(struct stk_camera *dev, u16 index, u8 value) int stk_camera_read_reg(struct stk_camera *dev, u16 index, int *value) { struct usb_device *udev = dev->udev; + unsigned char *buf; int ret; + buf = kmalloc(sizeof(u8), GFP_KERNEL); + if (!buf) + return -ENOMEM; + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), 0x00, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, 0x00, index, - (u8 *) value, + buf, sizeof(u8), 500); - if (ret < 0) - return ret; - else - return 0; + if (ret >= 0) + memcpy(value, buf, sizeof(u8)); + + kfree(buf); + return ret; } static int stk_start_stream(struct stk_camera *dev) -- 2.7.4 -- 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 v2 26/31] dw2102: return error if su3000_power_ctrl() fails
Instead of silently ignoring the error, return it. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dw2102.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/usb/dvb-usb/dw2102.c b/drivers/media/usb/dvb-usb/dw2102.c index 5fb0c650926e..2c720cb2fb00 100644 --- a/drivers/media/usb/dvb-usb/dw2102.c +++ b/drivers/media/usb/dvb-usb/dw2102.c @@ -852,7 +852,7 @@ static int su3000_power_ctrl(struct dvb_usb_device *d, int i) if (i && !state->initialized) { state->initialized = 1; /* reset board */ - dvb_usb_generic_rw(d, obuf, 2, NULL, 0, 0); + return dvb_usb_generic_rw(d, obuf, 2, NULL, 0, 0); } return 0; -- 2.7.4 -- 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 v2 01/31] af9005: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/af9005.c | 317 + 1 file changed, 180 insertions(+), 137 deletions(-) diff --git a/drivers/media/usb/dvb-usb/af9005.c b/drivers/media/usb/dvb-usb/af9005.c index efa782ed6e2d..4883f2fb4c2d 100644 --- a/drivers/media/usb/dvb-usb/af9005.c +++ b/drivers/media/usb/dvb-usb/af9005.c @@ -52,17 +52,16 @@ u8 regmask[8] = { 0x01, 0x03, 0x07, 0x0f, 0x1f, 0x3f, 0x7f, 0xff }; struct af9005_device_state { u8 sequence; int led_state; + unsigned char data[256]; + struct mutex data_mutex; }; static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg, int readwrite, int type, u8 * values, int len) { struct af9005_device_state *st = d->priv; - u8 obuf[16] = { 0 }; - u8 ibuf[17] = { 0 }; - u8 command; - int i; - int ret; + u8 command, seq; + int i, ret; if (len < 1) { err("generic read/write, less than 1 byte. Makes no sense."); @@ -73,16 +72,17 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg, return -EINVAL; } - obuf[0] = 14; /* rest of buffer length low */ - obuf[1] = 0;/* rest of buffer length high */ + mutex_lock(>data_mutex); + st->data[0] = 14; /* rest of buffer length low */ + st->data[1] = 0;/* rest of buffer length high */ - obuf[2] = AF9005_REGISTER_RW; /* register operation */ - obuf[3] = 12; /* rest of buffer length */ + st->data[2] = AF9005_REGISTER_RW; /* register operation */ + st->data[3] = 12; /* rest of buffer length */ - obuf[4] = st->sequence++; /* sequence number */ + st->data[4] = seq = st->sequence++; /* sequence number */ - obuf[5] = (u8) (reg >> 8); /* register address */ - obuf[6] = (u8) (reg & 0xff); + st->data[5] = (u8) (reg >> 8); /* register address */ + st->data[6] = (u8) (reg & 0xff); if (type == AF9005_OFDM_REG) { command = AF9005_CMD_OFDM_REG; @@ -96,51 +96,52 @@ static int af9005_generic_read_write(struct dvb_usb_device *d, u16 reg, command |= readwrite; if (readwrite == AF9005_CMD_WRITE) for (i = 0; i < len; i++) - obuf[8 + i] = values[i]; + st->data[8 + i] = values[i]; else if (type == AF9005_TUNER_REG) /* read command for tuner, the first byte contains the i2c address */ - obuf[8] = values[0]; - obuf[7] = command; + st->data[8] = values[0]; + st->data[7] = command; - ret = dvb_usb_generic_rw(d, obuf, 16, ibuf, 17, 0); + ret = dvb_usb_generic_rw(d, st->data, 16, st->data, 17, 0); if (ret) - return ret; + goto ret; /* sanity check */ - if (ibuf[2] != AF9005_REGISTER_RW_ACK) { + if (st->data[2] != AF9005_REGISTER_RW_ACK) { err("generic read/write, wrong reply code."); - return -EIO; + ret = -EIO; + goto ret; } - if (ibuf[3] != 0x0d) { + if (st->data[3] != 0x0d) { err("generic read/write, wrong length in reply."); - return -EIO; + ret = -EIO; + goto ret; } - if (ibuf[4] != obuf[4]) { + if (st->data[4] != seq) { err("generic read/write, wrong sequence in reply."); - return -EIO; + ret = -EIO; + goto ret; } /* - Windows driver doesn't check these fields, in fact sometimes - the register in the reply is different that what has been sent - - if (ibuf[5] != obuf[5] || ibuf[6] != obuf[6]) { - err("generic read/write, wrong register in reply."); - return -EIO; - } - if (ibuf[7] != command) { - err("generic read/write wrong command in reply."); - return -EIO; - } +* In thesis, both input and output buffers should have +* identical values for st->data[5] to st->data[8]. +* However, windows driver doesn't check these fields, in fact +* sometimes the register in the reply is different that what +* has been sent */ - if (ibuf[16] != 0x01) { + if (st->data[16] != 0x01) { err("generic read/write wrong status code in reply."); - return -EIO; + ret = -EIO; + goto ret; } + if (readwrite == AF9005_CMD_READ) for (i = 0; i < len;
[PATCH v2 28/31] cpia2_usb: don't use stack for DMA
From: Mauro Carvalho ChehabThe USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/usb/cpia2/cpia2_usb.c | 32 +--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/drivers/media/usb/cpia2/cpia2_usb.c b/drivers/media/usb/cpia2/cpia2_usb.c index 13620cdf0599..417d683b237d 100644 --- a/drivers/media/usb/cpia2/cpia2_usb.c +++ b/drivers/media/usb/cpia2/cpia2_usb.c @@ -545,10 +545,19 @@ static void free_sbufs(struct camera_data *cam) static int write_packet(struct usb_device *udev, u8 request, u8 * registers, u16 start, size_t size) { + unsigned char *buf; + int ret; + if (!registers || size <= 0) return -EINVAL; - return usb_control_msg(udev, + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + memcpy(buf, registers, size); + + ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), request, USB_TYPE_VENDOR | USB_RECIP_DEVICE, @@ -557,6 +566,9 @@ static int write_packet(struct usb_device *udev, registers, /* buffer */ size, HZ); + + kfree(buf); + return ret; } / @@ -567,18 +579,32 @@ static int write_packet(struct usb_device *udev, static int read_packet(struct usb_device *udev, u8 request, u8 * registers, u16 start, size_t size) { + unsigned char *buf; + int ret; + if (!registers || size <= 0) return -EINVAL; - return usb_control_msg(udev, + buf = kmalloc(size, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), request, USB_DIR_IN|USB_TYPE_VENDOR|USB_RECIP_DEVICE, start, /* value */ 0, /* index */ - registers, /* buffer */ + buf, /* buffer */ size, HZ); + + if (ret >= 0) + memcpy (registers, buf, size); + + kfree(buf); + + return ret; } /** -- 2.7.4 -- 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 v2 25/31] nova-t-usb2: handle error code on RC query
There's no sense on decoding and generating a RC key code if there was an error on the URB control message. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/nova-t-usb2.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/media/usb/dvb-usb/nova-t-usb2.c b/drivers/media/usb/dvb-usb/nova-t-usb2.c index 26d7188a1163..1babd3341910 100644 --- a/drivers/media/usb/dvb-usb/nova-t-usb2.c +++ b/drivers/media/usb/dvb-usb/nova-t-usb2.c @@ -76,7 +76,7 @@ static int nova_t_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { u8 *buf, data, toggle, custom; u16 raw; - int i; + int i, ret; struct dibusb_device_state *st = d->priv; buf = kmalloc(5, GFP_KERNEL); @@ -85,7 +85,9 @@ static int nova_t_rc_query(struct dvb_usb_device *d, u32 *event, int *state) buf[0] = DIBUSB_REQ_POLL_REMOTE; buf[1] = 0x35; - dvb_usb_generic_rw(d, buf, 2, buf, 5, 0); + ret = dvb_usb_generic_rw(d, buf, 2, buf, 5, 0); + if (ret < 0) + goto ret; *state = REMOTE_NO_KEY_PRESSED; switch (buf[0]) { @@ -124,8 +126,9 @@ static int nova_t_rc_query(struct dvb_usb_device *d, u32 *event, int *state) break; } +ret: kfree(buf); - return 0; + return ret; } static int nova_t_read_mac_address (struct dvb_usb_device *d, u8 mac[6]) -- 2.7.4 -- 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 v2 23/31] technisat-usb2: use DMA buffers for I2C transfers
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. On this driver, most of the transfers are OK, but the I2C one was using stack. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/technisat-usb2.c | 16 +++- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/media/usb/dvb-usb/technisat-usb2.c b/drivers/media/usb/dvb-usb/technisat-usb2.c index d9f3262bf071..4706628a3ed5 100644 --- a/drivers/media/usb/dvb-usb/technisat-usb2.c +++ b/drivers/media/usb/dvb-usb/technisat-usb2.c @@ -89,9 +89,13 @@ struct technisat_usb2_state { static int technisat_usb2_i2c_access(struct usb_device *udev, u8 device_addr, u8 *tx, u8 txlen, u8 *rx, u8 rxlen) { - u8 b[64]; + u8 *b; int ret, actual_length; + b = kmalloc(64, GFP_KERNEL); + if (!b) + return -ENOMEM; + deb_i2c("i2c-access: %02x, tx: ", device_addr); debug_dump(tx, txlen, deb_i2c); deb_i2c(" "); @@ -123,7 +127,7 @@ static int technisat_usb2_i2c_access(struct usb_device *udev, if (ret < 0) { err("i2c-error: out failed %02x = %d", device_addr, ret); - return -ENODEV; + goto err; } ret = usb_bulk_msg(udev, @@ -131,7 +135,7 @@ static int technisat_usb2_i2c_access(struct usb_device *udev, b, 64, _length, 1000); if (ret < 0) { err("i2c-error: in failed %02x = %d", device_addr, ret); - return -ENODEV; + goto err; } if (b[0] != I2C_STATUS_OK) { @@ -140,7 +144,7 @@ static int technisat_usb2_i2c_access(struct usb_device *udev, if (!(b[0] == I2C_STATUS_NAK && device_addr == 0x60 /* && device_is_technisat_usb2 */)) - return -ENODEV; + goto err; } deb_i2c("status: %d, ", b[0]); @@ -154,7 +158,9 @@ static int technisat_usb2_i2c_access(struct usb_device *udev, deb_i2c("\n"); - return 0; +err: + kfree(b); + return ret; } static int technisat_usb2_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, -- 2.7.4 -- 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 v2 27/31] digitv: handle error code on RC query
There's no sense on decoding and generating a RC key code if there was an error on the URB control message. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/digitv.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/media/usb/dvb-usb/digitv.c b/drivers/media/usb/dvb-usb/digitv.c index 09f8c28bd4db..4284f6984dc1 100644 --- a/drivers/media/usb/dvb-usb/digitv.c +++ b/drivers/media/usb/dvb-usb/digitv.c @@ -29,7 +29,9 @@ static int digitv_ctrl_msg(struct dvb_usb_device *d, u8 cmd, u8 vv, u8 *wbuf, int wlen, u8 *rbuf, int rlen) { struct digitv_state *st = d->priv; - int wo = (rbuf == NULL || rlen == 0); /* write-only */ + int ret, wo; + + wo = (rbuf == NULL || rlen == 0); /* write-only */ memset(st->sndbuf, 0, 7); memset(st->rcvbuf, 0, 7); @@ -40,12 +42,12 @@ static int digitv_ctrl_msg(struct dvb_usb_device *d, if (wo) { memcpy(>sndbuf[3], wbuf, wlen); - dvb_usb_generic_write(d, st->sndbuf, 7); + ret = dvb_usb_generic_write(d, st->sndbuf, 7); } else { - dvb_usb_generic_rw(d, st->sndbuf, 7, st->rcvbuf, 7, 10); + ret = dvb_usb_generic_rw(d, st->sndbuf, 7, st->rcvbuf, 7, 10); memcpy(rbuf, >rcvbuf[3], rlen); } - return 0; + return ret; } /* I2C */ -- 2.7.4 -- 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 v2 31/31] flexcop-usb: don't use stack for DMA
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. While here, remove a dead function calling usb_control_msg(). Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/b2c2/flexcop-usb.c | 105 +++ drivers/media/usb/b2c2/flexcop-usb.h | 4 ++ 2 files changed, 72 insertions(+), 37 deletions(-) diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c index d4bdba60b0f7..f26fc530564b 100644 --- a/drivers/media/usb/b2c2/flexcop-usb.c +++ b/drivers/media/usb/b2c2/flexcop-usb.c @@ -73,23 +73,34 @@ static int flexcop_usb_readwrite_dw(struct flexcop_device *fc, u16 wRegOffsPCI, u8 request_type = (read ? USB_DIR_IN : USB_DIR_OUT) | USB_TYPE_VENDOR; u8 wAddress = B2C2_FLEX_PCIOFFSET_TO_INTERNALADDR(wRegOffsPCI) | (read ? 0x80 : 0); + int ret; - int len = usb_control_msg(fc_usb->udev, + mutex_lock(_usb->data_mutex); + if (!read) + memcpy(fc_usb->data, val, sizeof(*val)); + + ret = usb_control_msg(fc_usb->udev, read ? B2C2_USB_CTRL_PIPE_IN : B2C2_USB_CTRL_PIPE_OUT, request, request_type, /* 0xc0 read or 0x40 write */ wAddress, 0, - val, + fc_usb->data, sizeof(u32), B2C2_WAIT_FOR_OPERATION_RDW * HZ); - if (len != sizeof(u32)) { + if (ret != sizeof(u32)) { err("error while %s dword from %d (%d).", read ? "reading" : "writing", wAddress, wRegOffsPCI); - return -EIO; + if (ret >= 0) + ret = -EIO; } - return 0; + + if (read && ret >= 0) + memcpy(val, fc_usb->data, sizeof(*val)); + mutex_unlock(_usb->data_mutex); + + return ret; } /* * DKT 010817 - add support for V8 memory read/write and flash update @@ -100,9 +111,14 @@ static int flexcop_usb_v8_memory_req(struct flexcop_usb *fc_usb, { u8 request_type = USB_TYPE_VENDOR; u16 wIndex; - int nWaitTime, pipe, len; + int nWaitTime, pipe, ret; wIndex = page << 8; + if (buflen > sizeof(fc_usb->data)) { + err("Buffer size bigger than max URB control message\n"); + return -EIO; + } + switch (req) { case B2C2_USB_READ_V8_MEM: nWaitTime = B2C2_WAIT_FOR_OPERATION_V8READ; @@ -127,17 +143,32 @@ static int flexcop_usb_v8_memory_req(struct flexcop_usb *fc_usb, deb_v8("v8mem: %02x %02x %04x %04x, len: %d\n", request_type, req, wAddress, wIndex, buflen); - len = usb_control_msg(fc_usb->udev, pipe, + mutex_lock(_usb->data_mutex); + + if ((request_type & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT) + memcpy(fc_usb->data, pbBuffer, buflen); + + ret = usb_control_msg(fc_usb->udev, pipe, req, request_type, wAddress, wIndex, - pbBuffer, + fc_usb->data, buflen, nWaitTime * HZ); + if (ret != buflen) + ret = -EIO; - debug_dump(pbBuffer, len, deb_v8); - return len == buflen ? 0 : -EIO; + if (ret >= 0) { + ret = 0; + if ((request_type & USB_ENDPOINT_DIR_MASK) == USB_DIR_IN) + memcpy(pbBuffer, fc_usb->data, buflen); + } + + mutex_unlock(_usb->data_mutex); + + debug_dump(pbBuffer, ret, deb_v8); + return ret; } #define bytes_left_to_read_on_page(paddr,buflen) \ @@ -196,29 +227,6 @@ static int flexcop_usb_get_mac_addr(struct flexcop_device *fc, int extended) fc->dvb_adapter.proposed_mac, 6); } -#if 0 -static int flexcop_usb_utility_req(struct flexcop_usb *fc_usb, int set, - flexcop_usb_utility_function_t func, u8 extra, u16 wIndex, - u16 buflen, u8 *pvBuffer) -{ - u16 wValue; - u8 request_type = (set ? USB_DIR_OUT : USB_DIR_IN) | USB_TYPE_VENDOR; - int nWaitTime = 2, - pipe = set ? B2C2_USB_CTRL_PIPE_OUT : B2C2_USB_CTRL_PIPE_IN, len; - wValue = (func << 8) | extra; - - len = usb_control_msg(fc_usb->udev,pipe, - B2C2_USB_UTILITY, - request_type, - wValue, - wIndex, - pvBuffer, - buflen, - nWaitTime * HZ); - return len == buflen ? 0 : -EIO; -} -#endif - /* usb i2c stuff */ static int flexcop_usb_i2c_req(struct flexcop_i2c_adapter *i2c,
[PATCH v2 29/31] s2255drv: don't use stack for DMA
From: Mauro Carvalho ChehabThe USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/usb/s2255/s2255drv.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c index c3a0e87066eb..f7bb78c1873c 100644 --- a/drivers/media/usb/s2255/s2255drv.c +++ b/drivers/media/usb/s2255/s2255drv.c @@ -1901,19 +1901,30 @@ static long s2255_vendor_req(struct s2255_dev *dev, unsigned char Request, s32 TransferBufferLength, int bOut) { int r; + unsigned char *buf; + + buf = kmalloc(TransferBufferLength, GFP_KERNEL); + if (!buf) + return -ENOMEM; + if (!bOut) { r = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), Request, USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN, - Value, Index, TransferBuffer, + Value, Index, buf, TransferBufferLength, HZ * 5); + + if (r >= 0) + memcpy(TransferBuffer, buf, TransferBufferLength); } else { + memcpy(buf, TransferBuffer, TransferBufferLength); r = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0), Request, USB_TYPE_VENDOR | USB_RECIP_DEVICE, - Value, Index, TransferBuffer, + Value, Index, buf, TransferBufferLength, HZ * 5); } + kfree(buf); return r; } -- 2.7.4 -- 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 v2 03/31] cinergyT2-core: handle error code on RC query
There's no sense on decoding and generating a RC key code if there was an error on the URB control message. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/cinergyT2-core.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c index d85c0c4d4042..2520e30f5338 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c @@ -102,7 +102,7 @@ static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap) /* Copy this pointer as we are gonna need it in the release phase */ cinergyt2_usb_device = adap->dev; - return 0; + return ret; } static struct rc_map_table rc_map_cinergyt2_table[] = { @@ -162,14 +162,16 @@ static int repeatable_keys[] = { static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { struct cinergyt2_state *st = d->priv; - int i; + int i, ret; *state = REMOTE_NO_KEY_PRESSED; mutex_lock(>data_mutex); st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; - dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + if (ret < 0) + return ret; if (st->data[4] == 0xff) { /* key repeat */ -- 2.7.4 -- 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 v2 08/31] dib0700_core: don't use stack on I2C reads
Be sure that I2C reads won't use stack by passing a pointer to the state buffer, that we know it was allocated via kmalloc, instead of relying on the buffer allocated by an I2C client. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/dib0700_core.c | 27 ++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c index 515f89dba199..92d5408684ac 100644 --- a/drivers/media/usb/dvb-usb/dib0700_core.c +++ b/drivers/media/usb/dvb-usb/dib0700_core.c @@ -213,7 +213,7 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg, usb_rcvctrlpipe(d->udev, 0), REQUEST_NEW_I2C_READ, USB_TYPE_VENDOR | USB_DIR_IN, -value, index, msg[i].buf, +value, index, st->buf, msg[i].len, USB_CTRL_GET_TIMEOUT); if (result < 0) { @@ -221,6 +221,14 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg, break; } + if (msg[i].len > sizeof(st->buf)) { + deb_info("buffer too small to fit %d bytes\n", +msg[i].len); + return -EIO; + } + + memcpy(msg[i].buf, st->buf, msg[i].len); + deb_data("<<< "); debug_dump(msg[i].buf, msg[i].len, deb_data); @@ -238,6 +246,13 @@ static int dib0700_i2c_xfer_new(struct i2c_adapter *adap, struct i2c_msg *msg, /* I2C ctrl + FE bus; */ st->buf[3] = ((gen_mode << 6) & 0xC0) | ((bus_mode << 4) & 0x30); + + if (msg[i].len > sizeof(st->buf) - 4) { + deb_info("i2c message to big: %d\n", +msg[i].len); + return -EIO; + } + /* The Actual i2c payload */ memcpy(>buf[4], msg[i].buf, msg[i].len); @@ -283,6 +298,11 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap, /* fill in the address */ st->buf[1] = msg[i].addr << 1; /* fill the buffer */ + if (msg[i].len > sizeof(st->buf) - 2) { + deb_info("i2c xfer to big: %d\n", + msg[i].len); + return -EIO; + } memcpy(>buf[2], msg[i].buf, msg[i].len); /* write/read request */ @@ -299,6 +319,11 @@ static int dib0700_i2c_xfer_legacy(struct i2c_adapter *adap, break; } + if (msg[i + 1].len > sizeof(st->buf)) { + deb_info("i2c xfer buffer to small for %d\n", + msg[i].len); + return -EIO; + } memcpy(msg[i + 1].buf, st->buf, msg[i + 1].len); msg[i+1].len = len; -- 2.7.4 -- 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 v2 09/31] dibusb: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/dibusb-common.c | 107 +++--- drivers/media/usb/dvb-usb/dibusb.h| 3 + 2 files changed, 85 insertions(+), 25 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dibusb-common.c b/drivers/media/usb/dvb-usb/dibusb-common.c index 4b08c2a47ae2..951f3dac9082 100644 --- a/drivers/media/usb/dvb-usb/dibusb-common.c +++ b/drivers/media/usb/dvb-usb/dibusb-common.c @@ -63,72 +63,117 @@ EXPORT_SYMBOL(dibusb_pid_filter_ctrl); int dibusb_power_ctrl(struct dvb_usb_device *d, int onoff) { - u8 b[3]; + u8 *b; int ret; + + b = kmalloc(3, GFP_KERNEL); + if (!b) + return -ENOMEM; + b[0] = DIBUSB_REQ_SET_IOCTL; b[1] = DIBUSB_IOCTL_CMD_POWER_MODE; b[2] = onoff ? DIBUSB_IOCTL_POWER_WAKEUP : DIBUSB_IOCTL_POWER_SLEEP; - ret = dvb_usb_generic_write(d,b,3); + + ret = dvb_usb_generic_write(d, b, 3); + + kfree(b); + msleep(10); + return ret; } EXPORT_SYMBOL(dibusb_power_ctrl); int dibusb2_0_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff) { - u8 b[3] = { 0 }; int ret; + u8 *b; + + b = kmalloc(3, GFP_KERNEL); + if (!b) + return -ENOMEM; if ((ret = dibusb_streaming_ctrl(adap,onoff)) < 0) - return ret; + goto ret; if (onoff) { b[0] = DIBUSB_REQ_SET_STREAMING_MODE; b[1] = 0x00; - if ((ret = dvb_usb_generic_write(adap->dev,b,2)) < 0) - return ret; + ret = dvb_usb_generic_write(adap->dev, b, 2); + if (ret < 0) + goto ret; } b[0] = DIBUSB_REQ_SET_IOCTL; b[1] = onoff ? DIBUSB_IOCTL_CMD_ENABLE_STREAM : DIBUSB_IOCTL_CMD_DISABLE_STREAM; - return dvb_usb_generic_write(adap->dev,b,3); + ret = dvb_usb_generic_write(adap->dev, b, 3); + +ret: + kfree(b); + return ret; } EXPORT_SYMBOL(dibusb2_0_streaming_ctrl); int dibusb2_0_power_ctrl(struct dvb_usb_device *d, int onoff) { - if (onoff) { - u8 b[3] = { DIBUSB_REQ_SET_IOCTL, DIBUSB_IOCTL_CMD_POWER_MODE, DIBUSB_IOCTL_POWER_WAKEUP }; - return dvb_usb_generic_write(d,b,3); - } else + u8 *b; + int ret; + + if (!onoff) return 0; + + b = kmalloc(3, GFP_KERNEL); + if (!b) + return -ENOMEM; + + b[0] = DIBUSB_REQ_SET_IOCTL; + b[1] = DIBUSB_IOCTL_CMD_POWER_MODE; + b[2] = DIBUSB_IOCTL_POWER_WAKEUP; + + ret = dvb_usb_generic_write(d, b, 3); + + kfree(b); + + return ret; } EXPORT_SYMBOL(dibusb2_0_power_ctrl); static int dibusb_i2c_msg(struct dvb_usb_device *d, u8 addr, u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen) { - u8 sndbuf[MAX_XFER_SIZE]; /* lead(1) devaddr,direction(1) addr(2) data(wlen) (len(2) (when reading)) */ + u8 *sndbuf; + int ret, wo, len; + /* write only ? */ - int wo = (rbuf == NULL || rlen == 0), - len = 2 + wlen + (wo ? 0 : 2); + wo = (rbuf == NULL || rlen == 0); - if (4 + wlen > sizeof(sndbuf)) { + len = 2 + wlen + (wo ? 0 : 2); + + sndbuf = kmalloc(MAX_XFER_SIZE, GFP_KERNEL); + if (!sndbuf) + return -ENOMEM; + + if (4 + wlen > MAX_XFER_SIZE) { warn("i2c wr: len=%d is too big!\n", wlen); - return -EOPNOTSUPP; + ret = -EOPNOTSUPP; + goto ret; } sndbuf[0] = wo ? DIBUSB_REQ_I2C_WRITE : DIBUSB_REQ_I2C_READ; sndbuf[1] = (addr << 1) | (wo ? 0 : 1); - memcpy([2],wbuf,wlen); + memcpy([2], wbuf, wlen); if (!wo) { - sndbuf[wlen+2] = (rlen >> 8) & 0xff; - sndbuf[wlen+3] = rlen & 0xff; + sndbuf[wlen + 2] = (rlen >> 8) & 0xff; + sndbuf[wlen + 3] = rlen & 0xff; } - return dvb_usb_generic_rw(d,sndbuf,len,rbuf,rlen,0); + ret = dvb_usb_generic_rw(d, sndbuf, len, rbuf, rlen, 0); + +ret: + kfree(sndbuf); + return ret; } /* @@ -320,11 +365,23 @@ EXPORT_SYMBOL(rc_map_dibusb_table); int dibusb_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { - u8 key[5],cmd = DIBUSB_REQ_POLL_REMOTE; - dvb_usb_generic_rw(d,,1,key,5,0); - dvb_usb_nec_rc_key_to_event(d,key,event,state); - if (key[0] != 0) - deb_info("key: %*ph\n", 5, key); + u8 *buf; + + buf = kmalloc(5, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + buf[0] = DIBUSB_REQ_POLL_REMOTE; + +
[PATCH v2 05/31] cinergyT2-fe: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/cinergyT2-fe.c | 56 +++- 1 file changed, 33 insertions(+), 23 deletions(-) diff --git a/drivers/media/usb/dvb-usb/cinergyT2-fe.c b/drivers/media/usb/dvb-usb/cinergyT2-fe.c index fd8edcb56e61..2d29b4174dba 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-fe.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-fe.c @@ -139,6 +139,10 @@ static uint16_t compute_tps(struct dtv_frontend_properties *op) struct cinergyt2_fe_state { struct dvb_frontend fe; struct dvb_usb_device *d; + + unsigned char data[64]; + struct mutex data_mutex; + struct dvbt_get_status_msg status; }; @@ -146,28 +150,31 @@ static int cinergyt2_fe_read_status(struct dvb_frontend *fe, enum fe_status *status) { struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_get_status_msg result; - u8 cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS }; int ret; - ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (u8 *), - sizeof(result), 0); + mutex_lock(>data_mutex); + state->data[0] = CINERGYT2_EP1_GET_TUNER_STATUS; + + ret = dvb_usb_generic_rw(state->d, state->data, 1, +state->data, sizeof(state->status), 0); + if (!ret) + memcpy(>status, state->data, sizeof(state->status)); + mutex_unlock(>data_mutex); + if (ret < 0) return ret; - state->status = result; - *status = 0; - if (0x - le16_to_cpu(result.gain) > 30) + if (0x - le16_to_cpu(state->status.gain) > 30) *status |= FE_HAS_SIGNAL; - if (result.lock_bits & (1 << 6)) + if (state->status.lock_bits & (1 << 6)) *status |= FE_HAS_LOCK; - if (result.lock_bits & (1 << 5)) + if (state->status.lock_bits & (1 << 5)) *status |= FE_HAS_SYNC; - if (result.lock_bits & (1 << 4)) + if (state->status.lock_bits & (1 << 4)) *status |= FE_HAS_CARRIER; - if (result.lock_bits & (1 << 1)) + if (state->status.lock_bits & (1 << 1)) *status |= FE_HAS_VITERBI; if ((*status & (FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC)) != @@ -232,34 +239,36 @@ static int cinergyt2_fe_set_frontend(struct dvb_frontend *fe) { struct dtv_frontend_properties *fep = >dtv_property_cache; struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_set_parameters_msg param; - char result[2]; + struct dvbt_set_parameters_msg *param; int err; - param.cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS; - param.tps = cpu_to_le16(compute_tps(fep)); - param.freq = cpu_to_le32(fep->frequency / 1000); - param.flags = 0; + mutex_lock(>data_mutex); + + param = (void *)state->data; + param->cmd = CINERGYT2_EP1_SET_TUNER_PARAMETERS; + param->tps = cpu_to_le16(compute_tps(fep)); + param->freq = cpu_to_le32(fep->frequency / 1000); + param->flags = 0; switch (fep->bandwidth_hz) { default: case 800: - param.bandwidth = 8; + param->bandwidth = 8; break; case 700: - param.bandwidth = 7; + param->bandwidth = 7; break; case 600: - param.bandwidth = 6; + param->bandwidth = 6; break; } - err = dvb_usb_generic_rw(state->d, - (char *), sizeof(param), - result, sizeof(result), 0); + err = dvb_usb_generic_rw(state->d, state->data, sizeof(*param), +state->data, 2, 0); if (err < 0) err("cinergyt2_fe_set_frontend() Failed! err=%d\n", err); + mutex_unlock(>data_mutex); return (err < 0) ? err : 0; } @@ -281,6 +290,7 @@ struct dvb_frontend *cinergyt2_fe_attach(struct dvb_usb_device *d) s->d = d; memcpy(>fe.ops, _fe_ops, sizeof(struct dvb_frontend_ops)); s->fe.demodulator_priv = s; + mutex_init(>data_mutex); return >fe; } -- 2.7.4 -- 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 v2 00/31] Don't use stack for DMA transers on media usb drivers
Sending URB control messages from stack was never supported. Yet, on x86, the stack was usually at a memory region that allows DMA transfer. So, several drivers got it wrong. On Kernel 4.9, if VMAP_STACK=y, none of those drivers will work, as the stack won't be on a DMA-able area anymore. So, fix the media drivers that requre it. It should noticed that the vast majority are at dvb-usb. Please notice that those patches are compile-tested only. So, I really appreciate if people with devices using those drivers could test and report if they don't break anything. I intend to send this PS.: I didn't verify any media driver inside staging. Version 2: - Added other USB drivers that are out of dvb-usb. After this series, all drivers will use kmalloc'ed buffers, although I won't doubt that some implementations may still have issues. - Be sure that, on all places where it is using a buffer allocated at the dvb-usb state structs, they'll be protected by some mutex. On some such cases, there was already a mutex at the DVB core. So, I didn't need to add a new one. Thanks to Antti that reminded me about such need. Mauro Carvalho Chehab (31): af9005: don't do DMA on stack cinergyT2-core: don't do DMA on stack cinergyT2-core: handle error code on RC query cinergyT2-fe: cache stats at cinergyt2_fe_read_status() cinergyT2-fe: don't do DMA on stack cxusb: don't do DMA on stack dib0700: be sure that dib0700_ctrl_rd() users can do DMA dib0700_core: don't use stack on I2C reads dibusb: don't do DMA on stack dibusb: handle error code on RC query digitv: don't do DMA on stack dtt200u-fe: don't keep waiting for lock at set_frontend() dtt200u-fe: don't do DMA on stack dtt200u-fe: handle errors on USB control messages dtt200u: don't do DMA on stack dtt200u: handle USB control message errors dtv5100: don't do DMA on stack gp8psk: don't do DMA on stack gp8psk: don't go past the buffer size nova-t-usb2: don't do DMA on stack pctv452e: don't do DMA on stack pctv452e: don't call BUG_ON() on non-fatal error technisat-usb2: use DMA buffers for I2C transfers dvb-usb: warn if return value for USB read/write routines is not checked nova-t-usb2: handle error code on RC query dw2102: return error if su3000_power_ctrl() fails digitv: handle error code on RC query cpia2_usb: don't use stack for DMA s2255drv: don't use stack for DMA stk-webcam: don't use stack for DMA flexcop-usb: don't use stack for DMA drivers/media/usb/b2c2/flexcop-usb.c| 105 + drivers/media/usb/b2c2/flexcop-usb.h| 4 + drivers/media/usb/cpia2/cpia2_usb.c | 32 ++- drivers/media/usb/dvb-usb/af9005.c | 317 drivers/media/usb/dvb-usb/cinergyT2-core.c | 90 +--- drivers/media/usb/dvb-usb/cinergyT2-fe.c| 100 - drivers/media/usb/dvb-usb/cxusb.c | 62 +++--- drivers/media/usb/dvb-usb/cxusb.h | 6 + drivers/media/usb/dvb-usb/dib0700_core.c| 31 ++- drivers/media/usb/dvb-usb/dib0700_devices.c | 25 +-- drivers/media/usb/dvb-usb/dibusb-common.c | 113 +++--- drivers/media/usb/dvb-usb/dibusb.h | 3 + drivers/media/usb/dvb-usb/digitv.c | 26 ++- drivers/media/usb/dvb-usb/digitv.h | 3 + drivers/media/usb/dvb-usb/dtt200u-fe.c | 128 +++ drivers/media/usb/dvb-usb/dtt200u.c | 120 --- drivers/media/usb/dvb-usb/dtv5100.c | 10 +- drivers/media/usb/dvb-usb/dvb-usb.h | 6 +- drivers/media/usb/dvb-usb/dw2102.c | 2 +- drivers/media/usb/dvb-usb/gp8psk.c | 25 ++- drivers/media/usb/dvb-usb/nova-t-usb2.c | 25 ++- drivers/media/usb/dvb-usb/pctv452e.c| 136 +++- drivers/media/usb/dvb-usb/technisat-usb2.c | 16 +- drivers/media/usb/s2255/s2255drv.c | 15 +- drivers/media/usb/stkwebcam/stk-webcam.c| 16 +- 25 files changed, 918 insertions(+), 498 deletions(-) -- 2.7.4 -- 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 v2 04/31] cinergyT2-fe: cache stats at cinergyt2_fe_read_status()
Instead of sending USB commands for every stats call, collect them once, when status is updated. As the frontend kthread will call it on every few seconds, the stats will still be collected. Besides reducing the amount of USB/I2C transfers, this also warrants that all stats will be collected at the same time, and makes easier to convert it to DVBv5 stats in the future. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/cinergyT2-fe.c | 48 +--- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/drivers/media/usb/dvb-usb/cinergyT2-fe.c b/drivers/media/usb/dvb-usb/cinergyT2-fe.c index b3ec743a7a2e..fd8edcb56e61 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-fe.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-fe.c @@ -139,6 +139,7 @@ static uint16_t compute_tps(struct dtv_frontend_properties *op) struct cinergyt2_fe_state { struct dvb_frontend fe; struct dvb_usb_device *d; + struct dvbt_get_status_msg status; }; static int cinergyt2_fe_read_status(struct dvb_frontend *fe, @@ -154,6 +155,8 @@ static int cinergyt2_fe_read_status(struct dvb_frontend *fe, if (ret < 0) return ret; + state->status = result; + *status = 0; if (0x - le16_to_cpu(result.gain) > 30) @@ -177,34 +180,16 @@ static int cinergyt2_fe_read_status(struct dvb_frontend *fe, static int cinergyt2_fe_read_ber(struct dvb_frontend *fe, u32 *ber) { struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_get_status_msg status; - char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS }; - int ret; - ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *), - sizeof(status), 0); - if (ret < 0) - return ret; - - *ber = le32_to_cpu(status.viterbi_error_rate); + *ber = le32_to_cpu(state->status.viterbi_error_rate); return 0; } static int cinergyt2_fe_read_unc_blocks(struct dvb_frontend *fe, u32 *unc) { struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_get_status_msg status; - u8 cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS }; - int ret; - ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (u8 *), - sizeof(status), 0); - if (ret < 0) { - err("cinergyt2_fe_read_unc_blocks() Failed! (Error=%d)\n", - ret); - return ret; - } - *unc = le32_to_cpu(status.uncorrected_block_count); + *unc = le32_to_cpu(state->status.uncorrected_block_count); return 0; } @@ -212,35 +197,16 @@ static int cinergyt2_fe_read_signal_strength(struct dvb_frontend *fe, u16 *strength) { struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_get_status_msg status; - char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS }; - int ret; - ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *), - sizeof(status), 0); - if (ret < 0) { - err("cinergyt2_fe_read_signal_strength() Failed!" - " (Error=%d)\n", ret); - return ret; - } - *strength = (0x - le16_to_cpu(status.gain)); + *strength = (0x - le16_to_cpu(state->status.gain)); return 0; } static int cinergyt2_fe_read_snr(struct dvb_frontend *fe, u16 *snr) { struct cinergyt2_fe_state *state = fe->demodulator_priv; - struct dvbt_get_status_msg status; - char cmd[] = { CINERGYT2_EP1_GET_TUNER_STATUS }; - int ret; - ret = dvb_usb_generic_rw(state->d, cmd, sizeof(cmd), (char *), - sizeof(status), 0); - if (ret < 0) { - err("cinergyt2_fe_read_snr() Failed! (Error=%d)\n", ret); - return ret; - } - *snr = (status.snr << 8) | status.snr; + *snr = (state->status.snr << 8) | state->status.snr; return 0; } -- 2.7.4 -- 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 v2 06/31] cxusb: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/cxusb.c | 62 ++- drivers/media/usb/dvb-usb/cxusb.h | 6 2 files changed, 41 insertions(+), 27 deletions(-) diff --git a/drivers/media/usb/dvb-usb/cxusb.c b/drivers/media/usb/dvb-usb/cxusb.c index 907ac01ae297..39772812269d 100644 --- a/drivers/media/usb/dvb-usb/cxusb.c +++ b/drivers/media/usb/dvb-usb/cxusb.c @@ -45,9 +45,6 @@ #include "si2168.h" #include "si2157.h" -/* Max transfer size done by I2C transfer functions */ -#define MAX_XFER_SIZE 80 - /* debug */ static int dvb_usb_cxusb_debug; module_param_named(debug, dvb_usb_cxusb_debug, int, 0644); @@ -61,23 +58,27 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); static int cxusb_ctrl_msg(struct dvb_usb_device *d, u8 cmd, u8 *wbuf, int wlen, u8 *rbuf, int rlen) { - int wo = (rbuf == NULL || rlen == 0); /* write-only */ - u8 sndbuf[MAX_XFER_SIZE]; + struct cxusb_state *st = d->priv; + int ret, wo; - if (1 + wlen > sizeof(sndbuf)) { - warn("i2c wr: len=%d is too big!\n", -wlen); + if (1 + wlen > MAX_XFER_SIZE) { + warn("i2c wr: len=%d is too big!\n", wlen); return -EOPNOTSUPP; } - memset(sndbuf, 0, 1+wlen); + wo = (rbuf == NULL || rlen == 0); /* write-only */ - sndbuf[0] = cmd; - memcpy([1], wbuf, wlen); + mutex_lock(>data_mutex); + st->data[0] = cmd; + memcpy(>data[1], wbuf, wlen); if (wo) - return dvb_usb_generic_write(d, sndbuf, 1+wlen); + ret = dvb_usb_generic_write(d, st->data, 1 + wlen); else - return dvb_usb_generic_rw(d, sndbuf, 1+wlen, rbuf, rlen, 0); + ret = dvb_usb_generic_rw(d, st->data, 1 + wlen, +rbuf, rlen, 0); + + mutex_unlock(>data_mutex); + return ret; } /* GPIO */ @@ -1460,36 +1461,43 @@ static struct dvb_usb_device_properties cxusb_mygica_t230_properties; static int cxusb_probe(struct usb_interface *intf, const struct usb_device_id *id) { + struct dvb_usb_device *d; + struct cxusb_state *st; + if (0 == dvb_usb_device_init(intf, _medion_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_lgh064f_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_dee1601_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_lgz201_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_dtt7579_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_dualdig4_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_nano2_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_nano2_needsfirmware_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _aver_a868r_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _bluebird_dualdig4_rev2_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _d680_dmb_properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 == dvb_usb_device_init(intf, _mygica_d689_properties, -
[PATCH v2 15/31] dtt200u: don't do DMA on stack
From: Mauro Carvalho ChehabThe USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick Boettcher Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/dtt200u.c | 104 ++-- 1 file changed, 75 insertions(+), 29 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c index d2a01b50af0d..7706938b5167 100644 --- a/drivers/media/usb/dvb-usb/dtt200u.c +++ b/drivers/media/usb/dvb-usb/dtt200u.c @@ -20,74 +20,103 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,xfer=2 (or-able))." DVB_USB DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); +struct dtt200u_state { + unsigned char data[80]; + struct mutex data_mutex; +}; + static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff) { - u8 b = SET_INIT; + struct dtt200u_state *st = d->priv; + + mutex_lock(>data_mutex); + + st->data[0] = SET_INIT; if (onoff) - dvb_usb_generic_write(d,,2); + dvb_usb_generic_write(d, st->data, 2); + mutex_unlock(>data_mutex); return 0; } static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff) { - u8 b_streaming[2] = { SET_STREAMING, onoff }; - u8 b_rst_pid = RESET_PID_FILTER; + struct dtt200u_state *st = adap->dev->priv; - dvb_usb_generic_write(adap->dev, b_streaming, 2); + mutex_lock(>data_mutex); + st->data[0] = SET_STREAMING; + st->data[1] = onoff; + + dvb_usb_generic_write(adap->dev, st->data, 2); + + if (onoff) + return 0; + + st->data[0] = RESET_PID_FILTER; + dvb_usb_generic_write(adap->dev, st->data, 1); + + mutex_unlock(>data_mutex); - if (onoff == 0) - dvb_usb_generic_write(adap->dev, _rst_pid, 1); return 0; } static int dtt200u_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid, int onoff) { - u8 b_pid[4]; + struct dtt200u_state *st = adap->dev->priv; + int ret; + pid = onoff ? pid : 0; - b_pid[0] = SET_PID_FILTER; - b_pid[1] = index; - b_pid[2] = pid & 0xff; - b_pid[3] = (pid >> 8) & 0x1f; + mutex_lock(>data_mutex); + st->data[0] = SET_PID_FILTER; + st->data[1] = index; + st->data[2] = pid & 0xff; + st->data[3] = (pid >> 8) & 0x1f; - return dvb_usb_generic_write(adap->dev, b_pid, 4); + ret = dvb_usb_generic_write(adap->dev, st->data, 4); + mutex_unlock(>data_mutex); + + return ret; } static int dtt200u_rc_query(struct dvb_usb_device *d) { - u8 key[5],cmd = GET_RC_CODE; + struct dtt200u_state *st = d->priv; u32 scancode; - dvb_usb_generic_rw(d,,1,key,5,0); - if (key[0] == 1) { + mutex_lock(>data_mutex); + st->data[0] = GET_RC_CODE; + + dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + if (st->data[0] == 1) { enum rc_type proto = RC_TYPE_NEC; - scancode = key[1]; - if ((u8) ~key[1] != key[2]) { + scancode = st->data[1]; + if ((u8) ~st->data[1] != st->data[2]) { /* Extended NEC */ scancode = scancode << 8; - scancode |= key[2]; + scancode |= st->data[2]; proto = RC_TYPE_NECX; } scancode = scancode << 8; - scancode |= key[3]; + scancode |= st->data[3]; /* Check command checksum is ok */ - if ((u8) ~key[3] == key[4]) + if ((u8) ~st->data[3] == st->data[4]) rc_keydown(d->rc_dev, proto, scancode, 0); else rc_keyup(d->rc_dev); - } else if (key[0] == 2) { + } else if (st->data[0] == 2) { rc_repeat(d->rc_dev); } else { rc_keyup(d->rc_dev); } - if (key[0] != 0) - deb_info("key: %*ph\n", 5, key); + if (st->data[0] != 0) + deb_info("st->data: %*ph\n", 5, st->data); + mutex_unlock(>data_mutex); return 0; } @@ -106,17 +135,24 @@ static struct dvb_usb_device_properties wt220u_miglia_properties; static int dtt200u_usb_probe(struct usb_interface *intf, const struct usb_device_id *id) { + struct dvb_usb_device *d; + struct dtt200u_state *st; + if (0 == dvb_usb_device_init(intf, _properties, -THIS_MODULE, NULL, adapter_nr) || +THIS_MODULE, , adapter_nr) || 0 ==
[PATCH v2 17/31] dtv5100: don't do DMA on stack
From: Mauro Carvalho ChehabThe USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab Signed-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/dtv5100.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtv5100.c b/drivers/media/usb/dvb-usb/dtv5100.c index 3d11df41cac0..c60fb54f445f 100644 --- a/drivers/media/usb/dvb-usb/dtv5100.c +++ b/drivers/media/usb/dvb-usb/dtv5100.c @@ -31,9 +31,14 @@ module_param_named(debug, dvb_usb_dtv5100_debug, int, 0644); MODULE_PARM_DESC(debug, "set debugging level" DVB_USB_DEBUG_STATUS); DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); +struct dtv5100_state { + unsigned char data[80]; +}; + static int dtv5100_i2c_msg(struct dvb_usb_device *d, u8 addr, u8 *wbuf, u16 wlen, u8 *rbuf, u16 rlen) { + struct dtv5100_state *st = d->priv; u8 request; u8 type; u16 value; @@ -60,9 +65,10 @@ static int dtv5100_i2c_msg(struct dvb_usb_device *d, u8 addr, } index = (addr << 8) + wbuf[0]; + memcpy(st->data, rbuf, rlen); msleep(1); /* avoid I2C errors */ return usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0), request, - type, value, index, rbuf, rlen, + type, value, index, st->data, rlen, DTV5100_USB_TIMEOUT); } @@ -176,7 +182,7 @@ static struct dvb_usb_device_properties dtv5100_properties = { .caps = DVB_USB_IS_AN_I2C_ADAPTER, .usb_ctrl = DEVICE_SPECIFIC, - .size_of_priv = 0, + .size_of_priv = sizeof(struct dtv5100_state), .num_adapters = 1, .adapter = {{ -- 2.7.4 -- 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 v2 21/31] pctv452e: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/pctv452e.c | 129 --- 1 file changed, 74 insertions(+), 55 deletions(-) diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c index c05de1b088a4..58b685094904 100644 --- a/drivers/media/usb/dvb-usb/pctv452e.c +++ b/drivers/media/usb/dvb-usb/pctv452e.c @@ -97,13 +97,14 @@ struct pctv452e_state { u8 c; /* transaction counter, wraps around... */ u8 initialized; /* set to 1 if 0x15 has been sent */ u16 last_rc_key; + + unsigned char data[80]; }; static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, u8 *data, unsigned int write_len, unsigned int read_len) { struct pctv452e_state *state = (struct pctv452e_state *)d->priv; - u8 buf[64]; u8 id; unsigned int rlen; int ret; @@ -112,33 +113,36 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, u8 *data, BUG_ON(write_len > 64 - 4); BUG_ON(read_len > 64 - 4); + mutex_lock(>ca_mutex); id = state->c++; - buf[0] = SYNC_BYTE_OUT; - buf[1] = id; - buf[2] = cmd; - buf[3] = write_len; + state->data[0] = SYNC_BYTE_OUT; + state->data[1] = id; + state->data[2] = cmd; + state->data[3] = write_len; - memcpy(buf + 4, data, write_len); + memcpy(state->data + 4, data, write_len); rlen = (read_len > 0) ? 64 : 0; - ret = dvb_usb_generic_rw(d, buf, 4 + write_len, - buf, rlen, /* delay_ms */ 0); + ret = dvb_usb_generic_rw(d, state->data, 4 + write_len, + state->data, rlen, /* delay_ms */ 0); if (0 != ret) goto failed; ret = -EIO; - if (SYNC_BYTE_IN != buf[0] || id != buf[1]) + if (SYNC_BYTE_IN != state->data[0] || id != state->data[1]) goto failed; - memcpy(data, buf + 4, read_len); + memcpy(data, state->data + 4, read_len); + mutex_unlock(>ca_mutex); return 0; failed: err("CI error %d; %02X %02X %02X -> %*ph.", -ret, SYNC_BYTE_OUT, id, cmd, 3, buf); +ret, SYNC_BYTE_OUT, id, cmd, 3, state->data); + mutex_unlock(>ca_mutex); return ret; } @@ -405,52 +409,53 @@ static int pctv452e_i2c_msg(struct dvb_usb_device *d, u8 addr, u8 *rcv_buf, u8 rcv_len) { struct pctv452e_state *state = (struct pctv452e_state *)d->priv; - u8 buf[64]; u8 id; int ret; + mutex_lock(>ca_mutex); id = state->c++; ret = -EINVAL; if (snd_len > 64 - 7 || rcv_len > 64 - 7) goto failed; - buf[0] = SYNC_BYTE_OUT; - buf[1] = id; - buf[2] = PCTV_CMD_I2C; - buf[3] = snd_len + 3; - buf[4] = addr << 1; - buf[5] = snd_len; - buf[6] = rcv_len; + state->data[0] = SYNC_BYTE_OUT; + state->data[1] = id; + state->data[2] = PCTV_CMD_I2C; + state->data[3] = snd_len + 3; + state->data[4] = addr << 1; + state->data[5] = snd_len; + state->data[6] = rcv_len; - memcpy(buf + 7, snd_buf, snd_len); + memcpy(state->data + 7, snd_buf, snd_len); - ret = dvb_usb_generic_rw(d, buf, 7 + snd_len, - buf, /* rcv_len */ 64, + ret = dvb_usb_generic_rw(d, state->data, 7 + snd_len, + state->data, /* rcv_len */ 64, /* delay_ms */ 0); if (ret < 0) goto failed; /* TT USB protocol error. */ ret = -EIO; - if (SYNC_BYTE_IN != buf[0] || id != buf[1]) + if (SYNC_BYTE_IN != state->data[0] || id != state->data[1]) goto failed; /* I2C device didn't respond as expected. */ ret = -EREMOTEIO; - if (buf[5] < snd_len || buf[6] < rcv_len) + if (state->data[5] < snd_len || state->data[6] < rcv_len) goto failed; - memcpy(rcv_buf, buf + 7, rcv_len); + memcpy(rcv_buf, state->data + 7, rcv_len); + mutex_unlock(>ca_mutex); return rcv_len; failed: - err("I2C error %d; %02X %02X %02X %02X %02X -> " -"%02X %02X %02X %02X %02X.", + err("I2C error %d; %02X %02X %02X %02X %02X -> %*ph", ret, SYNC_BYTE_OUT, id, addr << 1, snd_len, rcv_len, -buf[0], buf[1], buf[4], buf[5], buf[6]); +7, state->data); + mutex_unlock(>ca_mutex); return ret; } @@ -499,8 +504,7 @@ static u32 pctv452e_i2c_func(struct i2c_adapter *adapter) static
[PATCH v2 02/31] cinergyT2-core: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/cinergyT2-core.c | 84 ++ 1 file changed, 61 insertions(+), 23 deletions(-) diff --git a/drivers/media/usb/dvb-usb/cinergyT2-core.c b/drivers/media/usb/dvb-usb/cinergyT2-core.c index 9fd1527494eb..d85c0c4d4042 100644 --- a/drivers/media/usb/dvb-usb/cinergyT2-core.c +++ b/drivers/media/usb/dvb-usb/cinergyT2-core.c @@ -41,6 +41,8 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); struct cinergyt2_state { u8 rc_counter; + unsigned char data[64]; + struct mutex data_mutex; }; /* We are missing a release hook with usb_device data */ @@ -50,33 +52,52 @@ static struct dvb_usb_device_properties cinergyt2_properties; static int cinergyt2_streaming_ctrl(struct dvb_usb_adapter *adap, int enable) { - char buf[] = { CINERGYT2_EP1_CONTROL_STREAM_TRANSFER, enable ? 1 : 0 }; - char result[64]; - return dvb_usb_generic_rw(adap->dev, buf, sizeof(buf), result, - sizeof(result), 0); + struct dvb_usb_device *d = adap->dev; + struct cinergyt2_state *st = d->priv; + int ret; + + mutex_lock(>data_mutex); + st->data[0] = CINERGYT2_EP1_CONTROL_STREAM_TRANSFER; + st->data[1] = enable ? 1 : 0; + + ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 64, 0); + mutex_unlock(>data_mutex); + + return ret; } static int cinergyt2_power_ctrl(struct dvb_usb_device *d, int enable) { - char buf[] = { CINERGYT2_EP1_SLEEP_MODE, enable ? 0 : 1 }; - char state[3]; - return dvb_usb_generic_rw(d, buf, sizeof(buf), state, sizeof(state), 0); + struct cinergyt2_state *st = d->priv; + int ret; + + mutex_lock(>data_mutex); + st->data[0] = CINERGYT2_EP1_SLEEP_MODE; + st->data[1] = enable ? 0 : 1; + + ret = dvb_usb_generic_rw(d, st->data, 2, st->data, 3, 0); + mutex_unlock(>data_mutex); + + return ret; } static int cinergyt2_frontend_attach(struct dvb_usb_adapter *adap) { - char query[] = { CINERGYT2_EP1_GET_FIRMWARE_VERSION }; - char state[3]; + struct dvb_usb_device *d = adap->dev; + struct cinergyt2_state *st = d->priv; int ret; adap->fe_adap[0].fe = cinergyt2_fe_attach(adap->dev); - ret = dvb_usb_generic_rw(adap->dev, query, sizeof(query), state, - sizeof(state), 0); + mutex_lock(>data_mutex); + st->data[0] = CINERGYT2_EP1_GET_FIRMWARE_VERSION; + + ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 3, 0); if (ret < 0) { deb_rc("cinergyt2_power_ctrl() Failed to retrieve sleep " "state info\n"); } + mutex_unlock(>data_mutex); /* Copy this pointer as we are gonna need it in the release phase */ cinergyt2_usb_device = adap->dev; @@ -141,13 +162,16 @@ static int repeatable_keys[] = { static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { struct cinergyt2_state *st = d->priv; - u8 key[5] = {0, 0, 0, 0, 0}, cmd = CINERGYT2_EP1_GET_RC_EVENTS; int i; *state = REMOTE_NO_KEY_PRESSED; - dvb_usb_generic_rw(d, , 1, key, sizeof(key), 0); - if (key[4] == 0xff) { + mutex_lock(>data_mutex); + st->data[0] = CINERGYT2_EP1_GET_RC_EVENTS; + + dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + + if (st->data[4] == 0xff) { /* key repeat */ st->rc_counter++; if (st->rc_counter > RC_REPEAT_DELAY) { @@ -157,31 +181,45 @@ static int cinergyt2_rc_query(struct dvb_usb_device *d, u32 *event, int *state) *event = d->last_event; deb_rc("repeat key, event %x\n", *event); - return 0; + goto ret; } } deb_rc("repeated key (non repeatable)\n"); } - return 0; + goto ret; } /* hack to pass checksum on the custom field */ - key[2] = ~key[1]; - dvb_usb_nec_rc_key_to_event(d, key, event, state); - if (key[0] != 0) { + st->data[2] = ~st->data[1]; + dvb_usb_nec_rc_key_to_event(d, st->data, event, state); + if (st->data[0] != 0) { if (*event != d->last_event) st->rc_counter = 0; - deb_rc("key: %*ph\n", 5, key); + deb_rc("key: %*ph\n", 5, st->data); } - return 0; + +ret: +
[PATCH v2 10/31] dibusb: handle error code on RC query
There's no sense on decoding and generating a RC key code if there was an error on the URB control message. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dibusb-common.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dibusb-common.c b/drivers/media/usb/dvb-usb/dibusb-common.c index 951f3dac9082..b0fd9a609352 100644 --- a/drivers/media/usb/dvb-usb/dibusb-common.c +++ b/drivers/media/usb/dvb-usb/dibusb-common.c @@ -366,6 +366,7 @@ EXPORT_SYMBOL(rc_map_dibusb_table); int dibusb_rc_query(struct dvb_usb_device *d, u32 *event, int *state) { u8 *buf; + int ret; buf = kmalloc(5, GFP_KERNEL); if (!buf) @@ -373,7 +374,9 @@ int dibusb_rc_query(struct dvb_usb_device *d, u32 *event, int *state) buf[0] = DIBUSB_REQ_POLL_REMOTE; - dvb_usb_generic_rw(d, buf, 1, buf, 5, 0); + ret = dvb_usb_generic_rw(d, buf, 1, buf, 5, 0); + if (ret < 0) + goto ret; dvb_usb_nec_rc_key_to_event(d, buf, event, state); @@ -382,6 +385,7 @@ int dibusb_rc_query(struct dvb_usb_device *d, u32 *event, int *state) kfree(buf); - return 0; +ret: + return ret; } EXPORT_SYMBOL(dibusb_rc_query); -- 2.7.4 -- 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 v2 18/31] gp8psk: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/gp8psk.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c index 5d0384dd45b5..fa215ad37f7b 100644 --- a/drivers/media/usb/dvb-usb/gp8psk.c +++ b/drivers/media/usb/dvb-usb/gp8psk.c @@ -24,6 +24,10 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,xfer=2,rc=4 (or-able))." DV DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); +struct gp8psk_state { + unsigned char data[80]; +}; + static int gp8psk_get_fw_version(struct dvb_usb_device *d, u8 *fw_vers) { return (gp8psk_usb_in_op(d, GET_FW_VERS, 0, 0, fw_vers, 6)); @@ -53,17 +57,19 @@ static void gp8psk_info(struct dvb_usb_device *d) int gp8psk_usb_in_op(struct dvb_usb_device *d, u8 req, u16 value, u16 index, u8 *b, int blen) { + struct gp8psk_state *st = d->priv; int ret = 0,try = 0; if ((ret = mutex_lock_interruptible(>usb_mutex))) return ret; while (ret >= 0 && ret != blen && try < 3) { + memcpy(st->data, b, blen); ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev,0), req, USB_TYPE_VENDOR | USB_DIR_IN, - value,index,b,blen, + value, index, st->data, blen, 2000); deb_info("reading number %d (ret: %d)\n",try,ret); try++; @@ -86,6 +92,7 @@ int gp8psk_usb_in_op(struct dvb_usb_device *d, u8 req, u16 value, u16 index, u8 int gp8psk_usb_out_op(struct dvb_usb_device *d, u8 req, u16 value, u16 index, u8 *b, int blen) { + struct gp8psk_state *st = d->priv; int ret; deb_xfer("out: req. %x, val: %x, ind: %x, buffer: ",req,value,index); @@ -94,11 +101,12 @@ int gp8psk_usb_out_op(struct dvb_usb_device *d, u8 req, u16 value, if ((ret = mutex_lock_interruptible(>usb_mutex))) return ret; + memcpy(st->data, b, blen); if (usb_control_msg(d->udev, usb_sndctrlpipe(d->udev,0), req, USB_TYPE_VENDOR | USB_DIR_OUT, - value,index,b,blen, + value,index, st->data, blen, 2000) != blen) { warn("usb out operation failed."); ret = -EIO; @@ -265,6 +273,8 @@ static struct dvb_usb_device_properties gp8psk_properties = { .usb_ctrl = CYPRESS_FX2, .firmware = "dvb-usb-gp8psk-01.fw", + .size_of_priv = sizeof(struct gp8psk_state), + .num_adapters = 1, .adapter = { { -- 2.7.4 -- 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 v2 14/31] dtt200u-fe: handle errors on USB control messages
If something goes wrong, return an error code, instead of assuming that everything went fine. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dtt200u-fe.c | 40 ++ 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtt200u-fe.c b/drivers/media/usb/dvb-usb/dtt200u-fe.c index 7f7f64be6353..f5c042baa254 100644 --- a/drivers/media/usb/dvb-usb/dtt200u-fe.c +++ b/drivers/media/usb/dvb-usb/dtt200u-fe.c @@ -27,11 +27,17 @@ static int dtt200u_fe_read_status(struct dvb_frontend *fe, enum fe_status *stat) { struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; mutex_lock(>data_mutex); state->data[0] = GET_TUNE_STATUS; - dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); + ret = dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); + if (ret < 0) { + *stat = 0; + mutex_unlock(>data_mutex); + return ret; + } switch (state->data[0]) { case 0x01: @@ -53,25 +59,30 @@ static int dtt200u_fe_read_status(struct dvb_frontend *fe, static int dtt200u_fe_read_ber(struct dvb_frontend* fe, u32 *ber) { struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; mutex_lock(>data_mutex); state->data[0] = GET_VIT_ERR_CNT; - dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); - *ber = (state->data[0] << 16) | (state->data[1] << 8) | state->data[2]; + ret = dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); + if (ret >= 0) + *ber = (state->data[0] << 16) | (state->data[1] << 8) | state->data[2]; mutex_unlock(>data_mutex); - return 0; + return ret; } static int dtt200u_fe_read_unc_blocks(struct dvb_frontend* fe, u32 *unc) { struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; mutex_lock(>data_mutex); state->data[0] = GET_RS_UNCOR_BLK_CNT; - dvb_usb_generic_rw(state->d, state->data, 1, state->data, 2, 0); + ret = dvb_usb_generic_rw(state->d, state->data, 1, state->data, 2, 0); + if (ret >= 0) + *unc = (state->data[0] << 8) | state->data[1]; mutex_unlock(>data_mutex); return ret; @@ -80,11 +91,14 @@ static int dtt200u_fe_read_unc_blocks(struct dvb_frontend* fe, u32 *unc) static int dtt200u_fe_read_signal_strength(struct dvb_frontend* fe, u16 *strength) { struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; mutex_lock(>data_mutex); state->data[0] = GET_AGC; - dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + ret = dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + if (ret >= 0) + *strength = (state->data[0] << 8) | state->data[0]; mutex_unlock(>data_mutex); return ret; @@ -93,11 +107,14 @@ static int dtt200u_fe_read_signal_strength(struct dvb_frontend* fe, u16 *strengt static int dtt200u_fe_read_snr(struct dvb_frontend* fe, u16 *snr) { struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; mutex_lock(>data_mutex); state->data[0] = GET_SNR; - dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + ret = dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + if (ret >= 0) + *snr = ~((state->data[0] << 8) | state->data[0]); mutex_unlock(>data_mutex); return ret; @@ -134,6 +151,7 @@ static int dtt200u_fe_set_frontend(struct dvb_frontend *fe) { struct dtv_frontend_properties *fep = >dtv_property_cache; struct dtt200u_fe_state *state = fe->demodulator_priv; + int ret; u16 freq = fep->frequency / 25; mutex_lock(>data_mutex); @@ -153,12 +171,16 @@ static int dtt200u_fe_set_frontend(struct dvb_frontend *fe) goto ret; } - dvb_usb_generic_write(state->d, state->data, 2); + ret = dvb_usb_generic_write(state->d, state->data, 2); + if (ret < 0) + goto ret; state->data[0] = SET_RF_FREQ; state->data[1] = freq & 0xff; state->data[2] = (freq >> 8) & 0xff; - dvb_usb_generic_write(state->d, state->data, 3); + ret = dvb_usb_generic_write(state->d, state->data, 3); + if (ret < 0) + goto ret; ret: mutex_unlock(>data_mutex); -- 2.7.4 -- 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 v2 11/31] digitv: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Reviewed-By: Patrick BoettcherSigned-off-by: Mauro Carvalho Chehab --- drivers/media/usb/dvb-usb/digitv.c | 20 +++- drivers/media/usb/dvb-usb/digitv.h | 3 +++ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/drivers/media/usb/dvb-usb/digitv.c b/drivers/media/usb/dvb-usb/digitv.c index 63134335c994..09f8c28bd4db 100644 --- a/drivers/media/usb/dvb-usb/digitv.c +++ b/drivers/media/usb/dvb-usb/digitv.c @@ -28,20 +28,22 @@ DVB_DEFINE_MOD_OPT_ADAPTER_NR(adapter_nr); static int digitv_ctrl_msg(struct dvb_usb_device *d, u8 cmd, u8 vv, u8 *wbuf, int wlen, u8 *rbuf, int rlen) { + struct digitv_state *st = d->priv; int wo = (rbuf == NULL || rlen == 0); /* write-only */ - u8 sndbuf[7],rcvbuf[7]; - memset(sndbuf,0,7); memset(rcvbuf,0,7); - sndbuf[0] = cmd; - sndbuf[1] = vv; - sndbuf[2] = wo ? wlen : rlen; + memset(st->sndbuf, 0, 7); + memset(st->rcvbuf, 0, 7); + + st->sndbuf[0] = cmd; + st->sndbuf[1] = vv; + st->sndbuf[2] = wo ? wlen : rlen; if (wo) { - memcpy([3],wbuf,wlen); - dvb_usb_generic_write(d,sndbuf,7); + memcpy(>sndbuf[3], wbuf, wlen); + dvb_usb_generic_write(d, st->sndbuf, 7); } else { - dvb_usb_generic_rw(d,sndbuf,7,rcvbuf,7,10); - memcpy(rbuf,[3],rlen); + dvb_usb_generic_rw(d, st->sndbuf, 7, st->rcvbuf, 7, 10); + memcpy(rbuf, >rcvbuf[3], rlen); } return 0; } diff --git a/drivers/media/usb/dvb-usb/digitv.h b/drivers/media/usb/dvb-usb/digitv.h index 908c09f4966b..cf104689bdff 100644 --- a/drivers/media/usb/dvb-usb/digitv.h +++ b/drivers/media/usb/dvb-usb/digitv.h @@ -6,6 +6,9 @@ struct digitv_state { int is_nxt6000; + +unsigned char sndbuf[7]; +unsigned char rcvbuf[7]; }; /* protocol (from usblogging and the SDK: -- 2.7.4 -- 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 v2 19/31] gp8psk: don't go past the buffer size
Add checks to avoid going out of the buffer. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/gp8psk.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c index fa215ad37f7b..a745cf636846 100644 --- a/drivers/media/usb/dvb-usb/gp8psk.c +++ b/drivers/media/usb/dvb-usb/gp8psk.c @@ -60,6 +60,9 @@ int gp8psk_usb_in_op(struct dvb_usb_device *d, u8 req, u16 value, u16 index, u8 struct gp8psk_state *st = d->priv; int ret = 0,try = 0; + if (blen > sizeof(st->data)) + return -EIO; + if ((ret = mutex_lock_interruptible(>usb_mutex))) return ret; @@ -98,6 +101,9 @@ int gp8psk_usb_out_op(struct dvb_usb_device *d, u8 req, u16 value, deb_xfer("out: req. %x, val: %x, ind: %x, buffer: ",req,value,index); debug_dump(b,blen,deb_xfer); + if (blen > sizeof(st->data)) + return -EIO; + if ((ret = mutex_lock_interruptible(>usb_mutex))) return ret; @@ -151,6 +157,11 @@ static int gp8psk_load_bcm4500fw(struct dvb_usb_device *d) err("failed to load bcm4500 firmware."); goto out_free; } + if (buflen > 64) { + err("firmare chunk size bigger than 64 bytes."); + goto out_free; + } + memcpy(buf, ptr, buflen); if (dvb_usb_generic_write(d, buf, buflen)) { err("failed to load bcm4500 firmware."); -- 2.7.4 -- 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 v2 13/31] dtt200u-fe: don't do DMA on stack
The USB control messages require DMA to work. We cannot pass a stack-allocated buffer, as it is not warranted that the stack would be into a DMA enabled area. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dtt200u-fe.c | 95 +++--- 1 file changed, 65 insertions(+), 30 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtt200u-fe.c b/drivers/media/usb/dvb-usb/dtt200u-fe.c index 9bb15f7b48db..7f7f64be6353 100644 --- a/drivers/media/usb/dvb-usb/dtt200u-fe.c +++ b/drivers/media/usb/dvb-usb/dtt200u-fe.c @@ -18,17 +18,22 @@ struct dtt200u_fe_state { struct dtv_frontend_properties fep; struct dvb_frontend frontend; + + unsigned char data[80]; + struct mutex data_mutex; }; static int dtt200u_fe_read_status(struct dvb_frontend *fe, enum fe_status *stat) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 st = GET_TUNE_STATUS, b[3]; - dvb_usb_generic_rw(state->d,,1,b,3,0); + mutex_lock(>data_mutex); + state->data[0] = GET_TUNE_STATUS; - switch (b[0]) { + dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); + + switch (state->data[0]) { case 0x01: *stat = FE_HAS_SIGNAL | FE_HAS_CARRIER | FE_HAS_VITERBI | FE_HAS_SYNC | FE_HAS_LOCK; @@ -41,51 +46,75 @@ static int dtt200u_fe_read_status(struct dvb_frontend *fe, *stat = 0; break; } + mutex_unlock(>data_mutex); return 0; } static int dtt200u_fe_read_ber(struct dvb_frontend* fe, u32 *ber) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 bw = GET_VIT_ERR_CNT,b[3]; - dvb_usb_generic_rw(state->d,,1,b,3,0); - *ber = (b[0] << 16) | (b[1] << 8) | b[2]; + + mutex_lock(>data_mutex); + state->data[0] = GET_VIT_ERR_CNT; + + dvb_usb_generic_rw(state->d, state->data, 1, state->data, 3, 0); + *ber = (state->data[0] << 16) | (state->data[1] << 8) | state->data[2]; + + mutex_unlock(>data_mutex); return 0; } static int dtt200u_fe_read_unc_blocks(struct dvb_frontend* fe, u32 *unc) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 bw = GET_RS_UNCOR_BLK_CNT,b[2]; - dvb_usb_generic_rw(state->d,,1,b,2,0); - *unc = (b[0] << 8) | b[1]; - return 0; + mutex_lock(>data_mutex); + state->data[0] = GET_RS_UNCOR_BLK_CNT; + + dvb_usb_generic_rw(state->d, state->data, 1, state->data, 2, 0); + + mutex_unlock(>data_mutex); + return ret; } static int dtt200u_fe_read_signal_strength(struct dvb_frontend* fe, u16 *strength) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 bw = GET_AGC, b; - dvb_usb_generic_rw(state->d,,1,,1,0); - *strength = (b << 8) | b; - return 0; + + mutex_lock(>data_mutex); + state->data[0] = GET_AGC; + + dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + + mutex_unlock(>data_mutex); + return ret; } static int dtt200u_fe_read_snr(struct dvb_frontend* fe, u16 *snr) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 bw = GET_SNR,br; - dvb_usb_generic_rw(state->d,,1,,1,0); - *snr = ~((br << 8) | br); - return 0; + + mutex_lock(>data_mutex); + state->data[0] = GET_SNR; + + dvb_usb_generic_rw(state->d, state->data, 1, state->data, 1, 0); + + mutex_unlock(>data_mutex); + return ret; } static int dtt200u_fe_init(struct dvb_frontend* fe) { struct dtt200u_fe_state *state = fe->demodulator_priv; - u8 b = SET_INIT; - return dvb_usb_generic_write(state->d,,1); + int ret; + + mutex_lock(>data_mutex); + state->data[0] = SET_INIT; + + ret = dvb_usb_generic_write(state->d, state->data, 1); + mutex_unlock(>data_mutex); + + return ret; } static int dtt200u_fe_sleep(struct dvb_frontend* fe) @@ -106,29 +135,34 @@ static int dtt200u_fe_set_frontend(struct dvb_frontend *fe) struct dtv_frontend_properties *fep = >dtv_property_cache; struct dtt200u_fe_state *state = fe->demodulator_priv; u16 freq = fep->frequency / 25; - u8 bwbuf[2] = { SET_BANDWIDTH, 0 },freqbuf[3] = { SET_RF_FREQ, 0, 0 }; + mutex_lock(>data_mutex); + state->data[0] = SET_BANDWIDTH; switch (fep->bandwidth_hz) { case 800: - bwbuf[1] = 8; + state->data[1] = 8; break; case 700: - bwbuf[1] = 7; + state->data[1] = 7; break; case 600: - bwbuf[1] = 6; + state->data[1] = 6; break; default: - return -EINVAL; + ret = -EINVAL; +
[PATCH v2 12/31] dtt200u-fe: don't keep waiting for lock at set_frontend()
It is up to the frontend kthread to wait for lock. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dtt200u-fe.c | 9 - 1 file changed, 9 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtt200u-fe.c b/drivers/media/usb/dvb-usb/dtt200u-fe.c index c09332bd99cb..9bb15f7b48db 100644 --- a/drivers/media/usb/dvb-usb/dtt200u-fe.c +++ b/drivers/media/usb/dvb-usb/dtt200u-fe.c @@ -105,8 +105,6 @@ static int dtt200u_fe_set_frontend(struct dvb_frontend *fe) { struct dtv_frontend_properties *fep = >dtv_property_cache; struct dtt200u_fe_state *state = fe->demodulator_priv; - int i; - enum fe_status st; u16 freq = fep->frequency / 25; u8 bwbuf[2] = { SET_BANDWIDTH, 0 },freqbuf[3] = { SET_RF_FREQ, 0, 0 }; @@ -130,13 +128,6 @@ static int dtt200u_fe_set_frontend(struct dvb_frontend *fe) freqbuf[2] = (freq >> 8) & 0xff; dvb_usb_generic_write(state->d,freqbuf,3); - for (i = 0; i < 30; i++) { - msleep(20); - dtt200u_fe_read_status(fe, ); - if (st & FE_TIMEDOUT) - continue; - } - return 0; } -- 2.7.4 -- 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 v2 16/31] dtt200u: handle USB control message errors
If something bad happens while an USB control message is transfered, return an error code. Signed-off-by: Mauro Carvalho Chehab--- drivers/media/usb/dvb-usb/dtt200u.c | 26 ++ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c index 7706938b5167..f88572c7ae7c 100644 --- a/drivers/media/usb/dvb-usb/dtt200u.c +++ b/drivers/media/usb/dvb-usb/dtt200u.c @@ -28,37 +28,42 @@ struct dtt200u_state { static int dtt200u_power_ctrl(struct dvb_usb_device *d, int onoff) { struct dtt200u_state *st = d->priv; + int ret = 0; mutex_lock(>data_mutex); st->data[0] = SET_INIT; if (onoff) - dvb_usb_generic_write(d, st->data, 2); + ret = dvb_usb_generic_write(d, st->data, 2); mutex_unlock(>data_mutex); - return 0; + return ret; } static int dtt200u_streaming_ctrl(struct dvb_usb_adapter *adap, int onoff) { struct dtt200u_state *st = adap->dev->priv; + int ret; mutex_lock(>data_mutex); st->data[0] = SET_STREAMING; st->data[1] = onoff; - dvb_usb_generic_write(adap->dev, st->data, 2); + ret = dvb_usb_generic_write(adap->dev, st->data, 2); + if (ret < 0) + goto ret; if (onoff) - return 0; + goto ret; st->data[0] = RESET_PID_FILTER; - dvb_usb_generic_write(adap->dev, st->data, 1); + ret = dvb_usb_generic_write(adap->dev, st->data, 1); +ret: mutex_unlock(>data_mutex); - return 0; + return ret; } static int dtt200u_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid, int onoff) @@ -84,11 +89,15 @@ static int dtt200u_rc_query(struct dvb_usb_device *d) { struct dtt200u_state *st = d->priv; u32 scancode; + int ret; mutex_lock(>data_mutex); st->data[0] = GET_RC_CODE; - dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + ret = dvb_usb_generic_rw(d, st->data, 1, st->data, 5, 0); + if (ret < 0) + goto ret; + if (st->data[0] == 1) { enum rc_type proto = RC_TYPE_NEC; @@ -116,8 +125,9 @@ static int dtt200u_rc_query(struct dvb_usb_device *d) if (st->data[0] != 0) deb_info("st->data: %*ph\n", 5, st->data); +ret: mutex_unlock(>data_mutex); - return 0; + return ret; } static int dtt200u_frontend_attach(struct dvb_usb_adapter *adap) -- 2.7.4 -- 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 00/26] Don't use stack for DMA transers on dvb-usb drivers
Em Mon, 10 Oct 2016 07:44:53 -0400 Michael Ira Krufkyescreveu: > Antti makes a very good point. If we consider a situation where we > are streaming data while concurrently checking frontend status and > polling for IR codes, some locking will certainly be required in all > of these drivers. > > -Mike Krufky > > On Mon, Oct 10, 2016 at 7:24 AM, Antti Palosaari wrote: > > Hello > > If you use usb buffers from the state you will need add lock in order to > > protect concurrent access to buffer. There may have multiple concurrent > > operations from rc-polling/demux/frontend. Lets say you are reading ber and > > it sets data to buffer (state), then context switch to remote controller > > polling => buffer in state is overwritten, then context is changed back to > > ber reading and now there is bad data. Indeed a mutex to protect it is required. I added it and re-submitted the series, with a few extra patches, fixing issues on some drivers that are outside dvb-usb. Feel free to review. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] reST-directive kernel-cmd / include contentent from scripts
Am 07.10.2016 um 07:56 schrieb Jani Nikula: > On Thu, 06 Oct 2016, Mauro Carvalho Chehab wrote: >> Em Thu, 06 Oct 2016 17:21:36 +0300 >> Jani Nikula escreveu: >>> We've seen what happens when we make it easy to add random scripts to >>> build documentation. We've worked hard to get rid of that. In my books, >>> one of the bigger points in favor of Sphinx over AsciiDoc(tor) was >>> getting rid of all the hacks required in the build. Things that broke in >>> subtle ways. >> >> I really can't see what scripts it get rids. > > Really? You don't see why the DocBook build was so fragile and difficult > to maintain? That scares me a bit, because then you will not have > learned why we should at all costs avoid adding random scripts to > produce documentation. For me, disassembling the DocBok build was hard and bothersome, I don't want this back. IMO: old hats are productive with perl and they won't adapt another interpreter language (like python) for scripting. This series -- the kernel-cmd -- directive avoid that they build fragile and difficult to maintain Makefile constructs, calling their perl scripts. Am 06.10.2016 um 16:21 schrieb Jani Nikula : > This is connected to the above: keeping documentation buildable with > sphinx-build directly will force you to avoid the Makefile hacks. Thats why I think, that the kernel-cmd directive is a more *straight- forward* solution, helps to **avoid** complexity while not everyone has to script in python ... > Case in point, parse-headers.pl was added for a specific need of media > documentation, and for the life of me I can't figure out by reading the > script what good, if any, it would be for gpu documentation. I call > *that* unmaintainable. If one adds a script like parse-headers.pl to the Documentation/sphinx folder, he/she also has to add a documentation to the kernel-documentation.rst If the kernel-cmd directive gets acked, I will add a description to kernel-documentation.rst and I request Mauro to document the parse-headers.pl also. But, let's hear what Jon says. -- Markus -- -- 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