Re: [PATCH 1/9] mm: Introduce new vm_insert_range API
On Fri, Nov 16, 2018 at 11:59 PM Mike Rapoport wrote:
>
> On Thu, Nov 15, 2018 at 09:15:30PM +0530, Souptick Joarder wrote:
> > Previouly drivers have their own way of mapping range of
> > kernel pages/memory into user vma and this was done by
> > invoking vm_insert_page() within a loop.
> >
> > As this pattern is common across different drivers, it can
> > be generalized by creating a new function and use it across
> > the drivers.
> >
> > vm_insert_range is the new API which will be used to map a
> > range of kernel memory/pages to user vma.
> >
> > Signed-off-by: Souptick Joarder
> > Reviewed-by: Matthew Wilcox
> > ---
> > include/linux/mm_types.h | 3 +++
> > mm/memory.c | 28
> > mm/nommu.c | 7 +++
> > 3 files changed, 38 insertions(+)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 5ed8f62..15ae24f 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -523,6 +523,9 @@ extern void tlb_gather_mmu(struct mmu_gather *tlb,
> > struct mm_struct *mm,
> > extern void tlb_finish_mmu(struct mmu_gather *tlb,
> > unsigned long start, unsigned long end);
> >
> > +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> > + struct page **pages, unsigned long page_count);
> > +
> > static inline void init_tlb_flush_pending(struct mm_struct *mm)
> > {
> > atomic_set(&mm->tlb_flush_pending, 0);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 15c417e..da904ed 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1478,6 +1478,34 @@ static int insert_page(struct vm_area_struct *vma,
> > unsigned long addr,
> > }
> >
> > /**
> > + * vm_insert_range - insert range of kernel pages into user vma
> > + * @vma: user vma to map to
> > + * @addr: target user address of this page
> > + * @pages: pointer to array of source kernel pages
> > + * @page_count: no. of pages need to insert into user vma
> > + *
> > + * This allows drivers to insert range of kernel pages they've allocated
> > + * into a user vma. This is a generic function which drivers can use
> > + * rather than using their own way of mapping range of kernel pages into
> > + * user vma.
>
> Please add the return value and context descriptions.
>
> > + */
> > +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> > + struct page **pages, unsigned long page_count)
> > +{
> > + unsigned long uaddr = addr;
> > + int ret = 0, i;
> > +
> > + for (i = 0; i < page_count; i++) {
> > + ret = vm_insert_page(vma, uaddr, pages[i]);
> > + if (ret < 0)
> > + return ret;
> > + uaddr += PAGE_SIZE;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > * vm_insert_page - insert single page into user vma
> > * @vma: user vma to map to
> > * @addr: target user address of this page
> > diff --git a/mm/nommu.c b/mm/nommu.c
> > index 749276b..d6ef5c7 100644
> > --- a/mm/nommu.c
> > +++ b/mm/nommu.c
> > @@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma,
> > unsigned long addr,
> > }
> > EXPORT_SYMBOL(vm_insert_page);
> >
> > +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> > + struct page **pages, unsigned long page_count)
> > +{
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL(vm_insert_range);
> > +
> > /*
> > * sys_brk() for the most part doesn't need the global kernel
> > * lock, except when an application is doing something nasty
> > --
> > 1.9.1
> >
>
> --
> Sincerely yours,
> Mike.
>
Sure I will wait for some time to get additional review comments and
add all of those requested changes in v2.
Any further feedback on driver changes as part of this patch series ?
cron job: media_tree daily build: WARNINGS
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: Sat Nov 17 05:00:11 CET 2018 media-tree git hash:fbe57dde7126d1b2712ab5ea93fb9d15f89de708 media_build git hash: a8aef9cea0a4a2f3ea86c0b37bd6a1378018c0c1 v4l-utils git hash: 2ee0d9434630281dbcbe47719a268044775ec7e6 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-2-amd64 linux-git-arm-at91: WARNINGS linux-git-arm-davinci: WARNINGS linux-git-arm-multi: WARNINGS linux-git-arm-pxa: WARNINGS linux-git-arm-stm32: WARNINGS linux-git-arm64: OK linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: WARNINGS linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.123-i686: OK linux-3.18.123-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.159-i686: OK linux-4.4.159-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.131-i686: OK linux-4.9.131-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.74-i686: OK linux-4.14.74-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.12-i686: OK linux-4.18.12-x86_64: OK linux-4.19.1-i686: OK linux-4.19.1-x86_64: OK linux-4.20-rc1-i686: OK linux-4.20-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v2 1/2] media: docs-rst: Document memory-to-memory video decoder interface
Le jeudi 15 novembre 2018 à 15:34 +0100, Hans Verkuil a écrit : > On 10/22/2018 04:48 PM, Tomasz Figa wrote: > > Due to complexity of the video decoding process, the V4L2 drivers of > > stateful decoder hardware require specific sequences of V4L2 API calls > > to be followed. These include capability enumeration, initialization, > > decoding, seek, pause, dynamic resolution change, drain and end of > > stream. > > > > Specifics of the above have been discussed during Media Workshops at > > LinuxCon Europe 2012 in Barcelona and then later Embedded Linux > > Conference Europe 2014 in Düsseldorf. The de facto Codec API that > > originated at those events was later implemented by the drivers we already > > have merged in mainline, such as s5p-mfc or coda. > > > > The only thing missing was the real specification included as a part of > > Linux Media documentation. Fix it now and document the decoder part of > > the Codec API. > > > > Signed-off-by: Tomasz Figa > > --- > > Documentation/media/uapi/v4l/dev-decoder.rst | 1082 + > > Documentation/media/uapi/v4l/devices.rst |1 + > > Documentation/media/uapi/v4l/pixfmt-v4l2.rst |5 + > > Documentation/media/uapi/v4l/v4l2.rst | 10 +- > > .../media/uapi/v4l/vidioc-decoder-cmd.rst | 40 +- > > Documentation/media/uapi/v4l/vidioc-g-fmt.rst | 14 + > > 6 files changed, 1137 insertions(+), 15 deletions(-) > > create mode 100644 Documentation/media/uapi/v4l/dev-decoder.rst > > > > diff --git a/Documentation/media/uapi/v4l/dev-decoder.rst > > b/Documentation/media/uapi/v4l/dev-decoder.rst > > new file mode 100644 > > index ..09c7a6621b8e > > --- /dev/null > > +++ b/Documentation/media/uapi/v4l/dev-decoder.rst > > @@ -0,0 +1,1082 @@ > > +.. -*- coding: utf-8; mode: rst -*- > > + > > +.. _decoder: > > + > > +* > > +Memory-to-memory Stateful Video Decoder Interface > > +* > > + > > +A stateful video decoder takes complete chunks of the bitstream (e.g. > > Annex-B > > +H.264/HEVC stream, raw VP8/9 stream) and decodes them into raw video > > frames in > > +display order. The decoder is expected not to require any additional > > information > > +from the client to process these buffers. > > + > > +Performing software parsing, processing etc. of the stream in the driver in > > +order to support this interface is strongly discouraged. In case such > > +operations are needed, use of the Stateless Video Decoder Interface (in > > +development) is strongly advised. > > + > > +Conventions and notation used in this document > > +== > > + > > +1. The general V4L2 API rules apply if not specified in this document > > + otherwise. > > + > > +2. The meaning of words “must”, “may”, “should”, etc. is as per RFC > > + 2119. > > + > > +3. All steps not marked “optional” are required. > > + > > +4. :c:func:`VIDIOC_G_EXT_CTRLS`, :c:func:`VIDIOC_S_EXT_CTRLS` may be used > > + interchangeably with :c:func:`VIDIOC_G_CTRL`, :c:func:`VIDIOC_S_CTRL`, > > + unless specified otherwise. > > + > > +5. Single-plane API (see spec) and applicable structures may be used > > + interchangeably with Multi-plane API, unless specified otherwise, > > + depending on decoder capabilities and following the general V4L2 > > + guidelines. > > + > > +6. i = [a..b]: sequence of integers from a to b, inclusive, i.e. i = > > + [0..2]: i = 0, 1, 2. > > + > > +7. Given an ``OUTPUT`` buffer A, A’ represents a buffer on the ``CAPTURE`` > > + queue containing data (decoded frame/stream) that resulted from > > processing > > + buffer A. > > + > > +.. _decoder-glossary: > > + > > +Glossary > > + > > + > > +CAPTURE > > + the destination buffer queue; for decoder, the queue of buffers > > containing > > + decoded frames; for encoder, the queue of buffers containing encoded > > + bitstream; ``V4L2_BUF_TYPE_VIDEO_CAPTURE or > > + ``V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE``; data are captured from the > > hardware > > + into ``CAPTURE`` buffers > > + > > +client > > + application client communicating with the decoder or encoder > > implementing > > + this interface > > + > > +coded format > > + encoded/compressed video bitstream format (e.g. H.264, VP8, etc.); see > > + also: raw format > > + > > +coded height > > + height for given coded resolution > > + > > +coded resolution > > + stream resolution in pixels aligned to codec and hardware requirements; > > + typically visible resolution rounded up to full macroblocks; > > + see also: visible resolution > > + > > +coded width > > + width for given coded resolution > > + > > +decode order > > + the order in which frames are decoded; may differ from display order if > > the > > + coded format includes a feature of frame reordering; for decoders, > > + ``OUTPUT`` buffers must be queued by the client in decode order
Re: [PATCH v2 2/2] media: docs-rst: Document memory-to-memory video encoder interface
Le lundi 12 novembre 2018 à 14:23 +0100, Hans Verkuil a écrit :
> On 10/22/2018 04:49 PM, Tomasz Figa wrote:
> > Due to complexity of the video encoding process, the V4L2 drivers of
> > stateful encoder hardware require specific sequences of V4L2 API calls
> > to be followed. These include capability enumeration, initialization,
> > encoding, encode parameters change, drain and reset.
> >
> > Specifics of the above have been discussed during Media Workshops at
> > LinuxCon Europe 2012 in Barcelona and then later Embedded Linux
> > Conference Europe 2014 in Düsseldorf. The de facto Codec API that
> > originated at those events was later implemented by the drivers we already
> > have merged in mainline, such as s5p-mfc or coda.
> >
> > The only thing missing was the real specification included as a part of
> > Linux Media documentation. Fix it now and document the encoder part of
> > the Codec API.
> >
> > Signed-off-by: Tomasz Figa
> > ---
> > Documentation/media/uapi/v4l/dev-encoder.rst | 579 ++
> > Documentation/media/uapi/v4l/devices.rst | 1 +
> > Documentation/media/uapi/v4l/pixfmt-v4l2.rst | 5 +
> > Documentation/media/uapi/v4l/v4l2.rst | 2 +
> > .../media/uapi/v4l/vidioc-encoder-cmd.rst | 38 +-
> > 5 files changed, 610 insertions(+), 15 deletions(-)
> > create mode 100644 Documentation/media/uapi/v4l/dev-encoder.rst
> >
> > diff --git a/Documentation/media/uapi/v4l/dev-encoder.rst
> > b/Documentation/media/uapi/v4l/dev-encoder.rst
> > new file mode 100644
> > index ..41139e5e48eb
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/dev-encoder.rst
> > @@ -0,0 +1,579 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _encoder:
> > +
> > +*
> > +Memory-to-memory Stateful Video Encoder Interface
> > +*
> > +
> > +A stateful video encoder takes raw video frames in display order and
> > encodes
> > +them into a bitstream. It generates complete chunks of the bitstream,
> > including
> > +all metadata, headers, etc. The resulting bitstream does not require any
> > +further post-processing by the client.
> > +
> > +Performing software stream processing, header generation etc. in the driver
> > +in order to support this interface is strongly discouraged. In case such
> > +operations are needed, use of the Stateless Video Encoder Interface (in
> > +development) is strongly advised.
> > +
> > +Conventions and notation used in this document
> > +==
> > +
> > +1. The general V4L2 API rules apply if not specified in this document
> > + otherwise.
> > +
> > +2. The meaning of words "must", "may", "should", etc. is as per RFC
> > + 2119.
> > +
> > +3. All steps not marked "optional" are required.
> > +
> > +4. :c:func:`VIDIOC_G_EXT_CTRLS`, :c:func:`VIDIOC_S_EXT_CTRLS` may be used
> > + interchangeably with :c:func:`VIDIOC_G_CTRL`, :c:func:`VIDIOC_S_CTRL`,
> > + unless specified otherwise.
> > +
> > +5. Single-plane API (see spec) and applicable structures may be used
> > + interchangeably with Multi-plane API, unless specified otherwise,
> > + depending on encoder capabilities and following the general V4L2
> > + guidelines.
> > +
> > +6. i = [a..b]: sequence of integers from a to b, inclusive, i.e. i =
> > + [0..2]: i = 0, 1, 2.
> > +
> > +7. Given an ``OUTPUT`` buffer A, A' represents a buffer on the ``CAPTURE``
> > + queue containing data (encoded frame/stream) that resulted from
> > processing
> > + buffer A.
>
> The same comments as I mentioned for the previous patch apply to this section.
>
> > +
> > +Glossary
> > +
> > +
> > +Refer to :ref:`decoder-glossary`.
>
> Ah, you refer to the same glossary. Then my comment about the source
> resolution
> terms is obviously wrong.
>
> I wonder if it wouldn't be better to split off the sections above into a
> separate
> HW codec intro section where you explain the differences between
> stateful/stateless
> encoders and decoders, and add the conventions and glossary.
>
> After that you have the three documents for each variant (later four when we
> get
> stateless encoders).
>
> Up to you, and it can be done later in a follow-up patch.
>
> > +
> > +State machine
> > +=
> > +
> > +.. kernel-render:: DOT
> > + :alt: DOT digraph of encoder state machine
> > + :caption: Encoder state machine
> > +
> > + digraph encoder_state_machine {
> > + node [shape = doublecircle, label="Encoding"] Encoding;
> > +
> > + node [shape = circle, label="Initialization"] Initialization;
> > + node [shape = circle, label="Stopped"] Stopped;
> > + node [shape = circle, label="Drain"] Drain;
> > + node [shape = circle, label="Reset"] Reset;
> > +
> > + node [shape = point]; qi
> > + qi -> Initialization [ label = "open()" ];
> > +
> > + Initialization -> Encoding [ label = "Bot
[PATCH] media: videobuf2-core: fix use after free in vb2_mmap
When we are using __find_plane_by_offset() to find the matching plane
number and the buffer, the queue is not locked. So, after we have
calculated the buffer number and assigned the pointer to vb, it can get
freed. And if that happens then we get a use-after-free when we try to
use this for the mmap and get the following calltrace:
[ 30.623259] Call Trace:
[ 30.623531] dump_stack+0x244/0x39d
[ 30.623914] ? dump_stack_print_info.cold.1+0x20/0x20
[ 30.624439] ? printk+0xa7/0xcf
[ 30.624777] ? kmsg_dump_rewind_nolock+0xe4/0xe4
[ 30.625265] print_address_description.cold.7+0x9/0x1ff
[ 30.625809] kasan_report.cold.8+0x242/0x309
[ 30.626263] ? vb2_mmap+0x712/0x790
[ 30.626637] __asan_report_load8_noabort+0x14/0x20
[ 30.627201] vb2_mmap+0x712/0x790
[ 30.627642] ? vb2_poll+0x1d0/0x1d0
[ 30.628060] vb2_fop_mmap+0x4b/0x70
[ 30.628458] v4l2_mmap+0x153/0x200
[ 30.628929] mmap_region+0xe85/0x1cd0
Lock the queue before we start finding the matching plane and buffer so
that there is no chance of the memory being freed while we are about
to use it.
Reported-by: syzbot+be93025dd45dccd89...@syzkaller.appspotmail.com
Signed-off-by: Sudip Mukherjee
---
drivers/media/common/videobuf2/videobuf2-core.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 975ff5669f72..a81320566e02 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2125,9 +2125,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct
*vma)
/*
* Find the plane corresponding to the offset passed by userspace.
*/
+ mutex_lock(&q->mmap_lock);
ret = __find_plane_by_offset(q, off, &buffer, &plane);
- if (ret)
+ if (ret) {
+ mutex_unlock(&q->mmap_lock);
return ret;
+ }
vb = q->bufs[buffer];
@@ -2138,12 +2141,12 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct
*vma)
*/
length = PAGE_ALIGN(vb->planes[plane].length);
if (length < (vma->vm_end - vma->vm_start)) {
+ mutex_unlock(&q->mmap_lock);
dprintk(1,
"MMAP invalid, as it would overflow buffer length\n");
return -EINVAL;
}
- mutex_lock(&q->mmap_lock);
ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
mutex_unlock(&q->mmap_lock);
if (ret)
--
2.11.0
kernel BUG at arch/x86/mm/physaddr.c:LINE! (2)
Hello, syzbot found the following crash on: HEAD commit:5929a1f0ff30 Merge tag 'riscv-for-linus-4.20-rc2' of git:/.. git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=137766a340 kernel config: https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5 dashboard link: https://syzkaller.appspot.com/bug?extid=6c0effb5877f6b0344e2 compiler: gcc (GCC) 8.0.1 20180413 (experimental) Unfortunately, I don't have any reproducer for this crash yet. IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+6c0effb5877f6b034...@syzkaller.appspotmail.com [ cut here ] kernel BUG at arch/x86/mm/physaddr.c:27! invalid opcode: [#1] PREEMPT SMP KASAN CPU: 0 PID: 8479 Comm: syz-executor1 Not tainted 4.20.0-rc2+ #113 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 RIP: 0010:__phys_addr+0xb5/0x120 arch/x86/mm/physaddr.c:27 Code: 08 4c 89 e3 31 ff 48 d3 eb 48 89 de e8 b4 bb 45 00 48 85 db 75 0f e8 7a ba 45 00 4c 89 e0 5b 41 5c 41 5d 5d c3 e8 6b ba 45 00 <0f> 0b e8 64 ba 45 00 48 c7 c7 10 20 47 89 48 b8 00 00 00 00 00 fc RSP: 0018:88815771f410 EFLAGS: 00010016 RAX: 0004 RBX: 0001 RCX: c9000fc04000 RDX: 0119 RSI: 8139cd75 RDI: 0007 RBP: 88815771f428 R08: 888157a101c0 R09: ed103b5c5b67 R10: ed103b5c5b67 R11: 8881dae2db3b R12: 40801396c000 R13: R14: 0010 R15: 88815771fab8 FS: 7f52131d8700() GS:8881dae0() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 00720f9c CR3: 0001b5073000 CR4: 001406f0 DR0: 2000 DR1: 2000 DR2: DR3: DR6: fffe0ff0 DR7: 0600 Call Trace: virt_to_head_page include/linux/mm.h:658 [inline] virt_to_cache mm/slab.c:399 [inline] kfree+0x7b/0x230 mm/slab.c:3813 vivid_vid_cap_s_selection+0x2c31/0x38e0 drivers/media/platform/vivid/vivid-vid-cap.c:1006 vidioc_s_selection+0xa4/0xc0 drivers/media/platform/vivid/vivid-core.c:352 v4l_s_selection+0xba/0x140 drivers/media/v4l2-core/v4l2-ioctl.c:2197 __video_do_ioctl+0x8b1/0x1050 drivers/media/v4l2-core/v4l2-ioctl.c:2853 video_usercopy+0x5c1/0x1760 drivers/media/v4l2-core/v4l2-ioctl.c:3035 video_ioctl2+0x2c/0x33 drivers/media/v4l2-core/v4l2-ioctl.c:3079 v4l2_ioctl+0x154/0x1b0 drivers/media/v4l2-core/v4l2-dev.c:364 vfs_ioctl fs/ioctl.c:46 [inline] file_ioctl fs/ioctl.c:509 [inline] do_vfs_ioctl+0x1de/0x1790 fs/ioctl.c:696 ksys_ioctl+0xa9/0xd0 fs/ioctl.c:713 __do_sys_ioctl fs/ioctl.c:720 [inline] __se_sys_ioctl fs/ioctl.c:718 [inline] __x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:718 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x457569 Code: fd b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 cb b3 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:7f52131d7c78 EFLAGS: 0246 ORIG_RAX: 0010 RAX: ffda RBX: 0003 RCX: 00457569 RDX: 2000 RSI: c040565f RDI: 0003 RBP: 0072bf00 R08: R09: R10: R11: 0246 R12: 7f52131d86d4 R13: 004c1f77 R14: 004d3090 R15: Modules linked in: ---[ end trace df884aa85ab852c0 ]--- RIP: 0010:__phys_addr+0xb5/0x120 arch/x86/mm/physaddr.c:27 Code: 08 4c 89 e3 31 ff 48 d3 eb 48 89 de e8 b4 bb 45 00 48 85 db 75 0f e8 7a ba 45 00 4c 89 e0 5b 41 5c 41 5d 5d c3 e8 6b ba 45 00 <0f> 0b e8 64 ba 45 00 48 c7 c7 10 20 47 89 48 b8 00 00 00 00 00 fc RSP: 0018:88815771f410 EFLAGS: 00010016 RAX: 0004 RBX: 0001 RCX: c9000fc04000 RDX: 0119 RSI: 8139cd75 RDI: 0007 RBP: 88815771f428 R08: 888157a101c0 R09: ed103b5c5b67 R10: ed103b5c5b67 R11: 8881dae2db3b R12: 40801396c000 R13: R14: 0010 R15: 88815771fab8 FS: 7f52131d8700() GS:8881dae0() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 00720f9c CR3: 0001b5073000 CR4: 001406f0 DR0: 2000 DR1: 2000 DR2: DR3: DR6: fffe0ff0 DR7: 0600 --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkal...@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with syzbot.
Re: [Ksummit-discuss] [RFC PATCH 2/3] MAINTAINERS, Handbook: Subsystem Profile
On 11/15/18 7:44 AM, Mauro Carvalho Chehab wrote: > > Anyway, RFC patch follows. > > - > > [PATCH] [RFC] Add a system profile description for media subsystem > > This RFC aligns with current Dan's proposal for having subsystem > specific ruleset stored at the Kernel tree. > > On this initial RFC, I opted to not add the reviewers e-mail > (adding just a "<>") as a boilerplate. If we decide keeping emails > there, I'll add them. Hi- Here are my comments. Hopefully the email addresses will be added. Just having names is a half-answer for contact info. > > Signed-off-by: Mauro Carvalho Chehab > > diff --git a/Documentation/media/subsystem-profile.rst > b/Documentation/media/subsystem-profile.rst > new file mode 100644 > index ..7a5d6f691d05 > --- /dev/null > +++ b/Documentation/media/subsystem-profile.rst > @@ -0,0 +1,186 @@ > +Media Subsystem Profile > +=== > + > +Overview > + > + > +The media subsystem cover support for a variety of devices: stream covers > +capture, analog and digital TV, cameras, remote controllers, HDMI CEC > +and media pipeline control. > + > +Both our userspace and Kernel APIs are documented and should be kept in > +sync with the API changes. It means that all patches that add new > +features to the subsystem should also bring changes to the corresponding > +API files. > + > +Also, patches for device drivers that changes the Open Firmware/Device > +Tree bindings should be reviewed by the Device Tree maintainers. > + > +Due to the size and wide scope of the media subsystem, our > +maintainership model is to have sub-maintainers that have a broad > +knowledge of an specific aspect of the subsystem. It is a of a specific > +sub-maintainers task to review the patches, providing feedback to users > +if the patches are following the subsystem rules and are properly using > +the media internal and external APIs. > + > +We have a set of compliance tools at https://git.linuxtv.org/v4l-utils.git/ > +that should be used in order to check if the drivers are properly > +implementing the media APIs. > + > +Patches for the media subsystem should be sent to the media mailing list > +at linux-media@vger.kernel.org as plain text only e-mail. emails with e-mail or email? Be consistent. (more below) > +HTML will be automacially rejected by the mail server. automatically > + > +Our workflow is heavily based on Patchwork, meaning that, once a patch > +is submitted, it should appear at: > + > + - https://patchwork.linuxtv.org/project/linux-media/list/ > + > +If it doesn't automatically appear there after a few minutes, then > +probably something got wrong on your submission. Please check if the > +email is in plain text only and if your emailer is not mangling with email > +whitespaces before complaining or submit it again. > + > +Core > + > + > +Documentation > ++ > + > +F: Documentation/media > + > +Kernelspace API headers > > + > +F: include/media/*.h > + > +Digital TV Core > > + > +F: drivers/media/dvb-core > + > +HDMI CEC Core > ++ > + > +F: drivers/media/cec > + > +Media Controller Core > ++ > + > +F: drivers/media/media-\*.[ch] > + > +Remote Controller Core > +++ > + > +F: drivers/media/rc/rc-core-priv.h > +F: drivers/media/rc/rc-ir-raw.c > +F: drivers/media/rc/rc-main.c > +F: drivers/media/rc/ir\*-decoder.c > +F: drivers/media/rc/lirc_dev.c > + > +Video4linux Core > + > + > +F: drivers/media/v4l2-core > + > +Patches or Pull requests > + > + > +All patches should be submitted via e-mail for review. We use and e-mail > +pull requests on our workflow between sub-maintainers and the > +maintainer. > + > + > +Last day for new feature submissions > + > + > +Before -rc5 > + > + > +Last day to merge features > +-- > + > +Before -rc7 > + > + > +Non-author Ack / Review Tags Required > +- > + > +Not required, but desirable > + > + > +Test Suite > +-- > + > +Use the several *-compliance tools that are part of the v4l-utils > +package. > + > + > +Trusted Reviewers > +- > + > +Sub-maintainers > > + > +At the media subsystem, we have a group of senior developers that are > +responsible for doing the code reviews at the drivers (called > +sub-maintainers), and another senior developer responsible for the > +subsystem as a hole. For core changes, whenever possible, multiple as a whole. > +media (sub-)maintainers do the review. > + > +The sub-maintainers work on specific areas of the subsystem, as > +described below: > + > +- Sensor drivers > + > + R: Sakari Ailus <> > + > +- V4L2 drivers > + > + R: Hans Verkuil <> > + > +- Media controller drivers > + > + R: Laurent Pinc
RE: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
Hi, Sakari,
Thanks for the thorough review.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, November 2, 2018 8:03 AM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> mche...@kernel.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu ; Li, Chao C
> Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
>
> Hi Yong,
>
> Thanks for the update! I went through this again... a few comments below
> but I'd say they're mostly pretty minor issues.
>
> On Mon, Oct 29, 2018 at 03:22:57PM -0700, Yong Zhi wrote:
> > These meta formats are used on Intel IPU3 ImgU video queues
> > to carry 3A statistics and ISP pipeline parameters.
> >
> > V4L2_META_FMT_IPU3_3A
> > V4L2_META_FMT_IPU3_PARAMS
> >
> > Signed-off-by: Yong Zhi
> > Signed-off-by: Chao C Li
> > Signed-off-by: Rajmohan Mani
> > ---
> > Documentation/media/uapi/v4l/meta-formats.rst |1 +
> > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 181 ++
> > include/uapi/linux/intel-ipu3.h| 2819
> >
> > 3 files changed, 3001 insertions(+)
> > create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-intel-
> ipu3.rst
> > create mode 100644 include/uapi/linux/intel-ipu3.h
> >
> > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst
> > index cf971d5..eafc534 100644
> > --- a/Documentation/media/uapi/v4l/meta-formats.rst
> > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > @@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata`
> interface only.
> > .. toctree::
> > :maxdepth: 1
> >
> > +pixfmt-meta-intel-ipu3
> > pixfmt-meta-d4xx
> > pixfmt-meta-uvc
> > pixfmt-meta-vsp1-hgo
> > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > new file mode 100644
> > index 000..23b945b
> > --- /dev/null
> > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst
> > @@ -0,0 +1,181 @@
> > +.. -*- coding: utf-8; mode: rst -*-
> > +
> > +.. _intel-ipu3:
>
> Instead, to avoid a warning from Sphinx, replace the line with these:
>
> .. _v4l2-meta-fmt-ipu3-params:
> .. _v4l2-meta-fmt-ipu3-stat-3a:
>
Ack.
> > +
> >
> +***
> ***
> > +V4L2_META_FMT_IPU3_PARAMS ('ip3p'), V4L2_META_FMT_IPU3_3A
> ('ip3s')
> >
> +***
> ***
> > +
> > +.. c:type:: ipu3_uapi_stats_3a
> > +
> > +3A statistics
> > +=
> > +
> > +For IPU3 ImgU, the 3A statistics accelerators collect different statistics
> > over
> > +an input bayer frame. Those statistics, defined in data struct
> > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained from "ipu3-imgu
> 3a stat"
> > +video node, which are then passed to user space for statistics analysis
> > +using :c:type:`v4l2_meta_format` interface.
> > +
> > +The statistics collected are AWB (Auto-white balance) RGBS (Red, Green,
> Blue and
>
> Extra whitespace at the end of the line.
>
Ack.
> > +Saturation measure) cells, AWB filter response, AF (Auto-focus) filter
> response,
> > +and AE (Auto-exposure) histogram.
> > +
> > +struct :c:type:`ipu3_uapi_4a_config` saves configurable parameters for all
> above.
> > +
> > +
> > +.. code-block:: c
> > +
> > +
> > + struct ipu3_uapi_stats_3a {
> > + struct ipu3_uapi_awb_raw_buffer awb_raw_buffer
> > +__attribute__((aligned(32)));
> > + struct ipu3_uapi_ae_raw_buffer_aligned
> > + ae_raw_buffer[IPU3_UAPI_MAX_STRIPES];
> > + struct ipu3_uapi_af_raw_buffer af_raw_buffer;
> > + struct ipu3_uapi_awb_fr_raw_buffer awb_fr_raw_buffer;
> > + struct ipu3_uapi_4a_config stats_4a_config;
> > + __u32 ae_join_buffers;
> > + __u8 padding[28];
> > + struct ipu3_uapi_stats_3a_bubble_info_per_stripe
> > + stats_3a_bubble_per_stripe;
>
> I think you could just unwrap these, even if it causes them to be over 80
> characters per line. They display better in a web browser that way. Or
> alternatively align the wrapped lines to the same column.
>
Ack.
> > + struct ipu3_uapi_ff_status stats_3a_status;
> > + } __packed;
> > +
> > +
> > +.. c:type:: ipu3_uapi_params
> > +
> > +Pipeline parameters
> > +===
> > +
> > +IPU3 pipeline has a number of image processing stages, each of which
> takes a
> > +set of parameters as input. The major stages of pipelines are shown here:
> > +
> > +Raw pixels -> Bayer Downscaling -> Optical Black Correction ->
> > +
> > +Linearization -> Lens Shading Correction -> White Balance / Exposure /
> > +
> > +Focus Apply -> Bayer Noise Reduction -> ANR -> Demosaicing -> Color
> > +
> > +Correction Matrix -> Gamma correction -> Color Space Conversio
possible deadlock in v4l2_release
Hello,
syzbot found the following crash on:
HEAD commit:5929a1f0ff30 Merge tag 'riscv-for-linus-4.20-rc2' of git:/..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1443d26d40
kernel config: https://syzkaller.appspot.com/x/.config?x=4a0a89f12ca9b0f5
dashboard link: https://syzkaller.appspot.com/bug?extid=ea05c832a73d0615bf33
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+ea05c832a73d0615b...@syzkaller.appspotmail.com
netlink: 'syz-executor0': attribute type 1 has an invalid length.
netlink: 4 bytes leftover after parsing attributes in process
`syz-executor0'.
==
WARNING: possible circular locking dependency detected
kobject: 'loop0' (9dc3fb20): kobject_uevent_env
4.20.0-rc2+ #335 Not tainted
--
kworker/1:0/17 is trying to acquire lock:
651ab15a (&mdev->req_queue_mutex){+.+.}, at:
v4l2_release+0x1d7/0x3a0 drivers/media/v4l2-core/v4l2-dev.c:455
but task is already holding lock:
kobject: 'loop0' (9dc3fb20): fill_kobj_path: path
= '/devices/virtual/block/loop0'
1fd67464 ((delayed_fput_work).work){+.+.}, at:
process_one_work+0xb9a/0x1c40 kernel/workqueue.c:2128
which lock already depends on the new lock.
netlink: 'syz-executor0': attribute type 1 has an invalid length.
the existing dependency chain (in reverse order) is:
-> #3 ((delayed_fput_work).work){+.+.}:
process_one_work+0xc0a/0x1c40 kernel/workqueue.c:2129
worker_thread+0x17f/0x1390 kernel/workqueue.c:2296
kthread+0x35a/0x440 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
kobject: 'loop5' (e380a391): kobject_uevent_env
-> #2 ((wq_completion)"events"){+.+.}:
flush_workqueue+0x30a/0x1e10 kernel/workqueue.c:2655
flush_scheduled_work include/linux/workqueue.h:599 [inline]
vim2m_stop_streaming+0x7c/0x2c0 drivers/media/platform/vim2m.c:811
__vb2_queue_cancel+0x171/0xd20
drivers/media/common/videobuf2/videobuf2-core.c:1823
kobject: 'loop5' (e380a391): fill_kobj_path: path
= '/devices/virtual/block/loop5'
vb2_core_queue_release+0x26/0x80
drivers/media/common/videobuf2/videobuf2-core.c:2229
vb2_queue_release+0x15/0x20
drivers/media/common/videobuf2/videobuf2-v4l2.c:837
v4l2_m2m_ctx_release+0x1e/0x35
drivers/media/v4l2-core/v4l2-mem2mem.c:930
netlink: 4 bytes leftover after parsing attributes in process
`syz-executor0'.
vim2m_release+0xe6/0x150 drivers/media/platform/vim2m.c:977
v4l2_release+0x224/0x3a0 drivers/media/v4l2-core/v4l2-dev.c:456
__fput+0x385/0xa30 fs/file_table.c:278
fput+0x15/0x20 fs/file_table.c:309
kobject: 'loop0' (9dc3fb20): kobject_uevent_env
task_work_run+0x1e8/0x2a0 kernel/task_work.c:113
tracehook_notify_resume include/linux/tracehook.h:188 [inline]
exit_to_usermode_loop+0x318/0x380 arch/x86/entry/common.c:166
kobject: 'loop0' (9dc3fb20): fill_kobj_path: path
= '/devices/virtual/block/loop0'
prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
do_syscall_64+0x6be/0x820 arch/x86/entry/common.c:293
entry_SYSCALL_64_after_hwframe+0x49/0xbe
-> #1 (&dev->dev_mutex){+.+.}:
__mutex_lock_common kernel/locking/mutex.c:925 [inline]
__mutex_lock+0x166/0x16f0 kernel/locking/mutex.c:1072
mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:1087
vim2m_release+0xbc/0x150 drivers/media/platform/vim2m.c:976
v4l2_release+0x224/0x3a0 drivers/media/v4l2-core/v4l2-dev.c:456
__fput+0x385/0xa30 fs/file_table.c:278
fput+0x15/0x20 fs/file_table.c:309
task_work_run+0x1e8/0x2a0 kernel/task_work.c:113
tracehook_notify_resume include/linux/tracehook.h:188 [inline]
exit_to_usermode_loop+0x318/0x380 arch/x86/entry/common.c:166
prepare_exit_to_usermode arch/x86/entry/common.c:197 [inline]
syscall_return_slowpath arch/x86/entry/common.c:268 [inline]
do_syscall_64+0x6be/0x820 arch/x86/entry/common.c:293
entry_SYSCALL_64_after_hwframe+0x49/0xbe
kobject: 'loop2' (1e25d2fb): kobject_uevent_env
-> #0 (&mdev->req_queue_mutex){+.+.}:
kobject: 'loop2' (1e25d2fb): fill_kobj_path: path
= '/devices/virtual/block/loop2'
lock_acquire+0x1ed/0x520 kernel/locking/lockdep.c:3844
__mutex_lock_common kernel/locking/mutex.c:925 [inline]
__mutex_lock+0x166/0x16f0 kernel/locking/mutex.c:1072
mutex_lock_nested+0x16/0x20 kernel/locking/mutex.c:1087
v4l2_release+0x1d7/0x3a0 drivers/media/v4l2-core/v4l2-dev.c:455
__fput+0x385/0xa30 fs/file_table.c:278
delayed_fput+0x55/0x
Re: TechnoTrend CT2-4500 remote not working
Sean, Am 16.11.2018 um 12:26 schrieb Sean Young: Ok, thank you. Now, we don't know how the IR is wired up. Please could you try enabling the enable_885_ir module parameter for cx23885. If this goes badly, then we might end up in an infinite loop of unending interrupts, so it would be prudent to not change your startup scripts to set this. As root, please run: rmmod cx23885 modprobe cx23885 enable_885_ir=1 Thank you, loading the module with enable_885_ir=1 works: cx23885: cx23885_dev_checkrevision() Hardware revision = 0xa5 cx23885: cx23885[0]/0: found at :17:00.0, rev: 4, irq: 31, latency: 0, mmio: 0xfe00 Registered IR keymap rc-tt-1500 IR RC5(x/sz) protocol handler initialized rc rc1: cx23885 IR (Technotrend TT-budget CT2-4500 CI) as /devices/pci:00/:00:01.2/:15:00.2/:16:00.0/:17:00.0/rc/rc1 input: cx23885 IR (Technotrend TT-budget CT2-4500 CI) as /devices/pci:00/:00:01.2/:15:00.2/:16:00.0/:17:00.0/rc/rc1/input21 ir-keytable output: Found /sys/class/rc/rc1/ (/dev/input/event17) with: Name: cx23885 IR (Technotrend TT-budget CT2-4500 CI) Driver: cx23885, table: rc-tt-1500 lirc device: /dev/lirc1 Supported protocols: other lirc rc-5 rc-5-sz jvc sony nec sanyo mce_kbd rc-6 sharp xmp Enabled protocols: lirc rc-5 bus: 1, vendor/product: 13c2:3013, version: 0x0001 Repeat delay = 500 ms, repeat period = 125 ms Using ir-keytable -t I see all the buttons pressed. Maybe you can amend the module options description to include my device: parm: enable_885_ir:Enable integrated IR controller for supported CX2388[57] boards that are wired for it: HVR-1250 (reported safe) TerraTec Cinergy T PCIe Dual (not well tested, appears to be safe) TeVii S470 (reported unsafe) This can cause an interrupt storm with some cards. Default: 0 [Disabled] (int) You should get another rc device, which might just work. Thanks again Sean for the help! Martin
Re: [PATCH 1/9] mm: Introduce new vm_insert_range API
On Thu, Nov 15, 2018 at 09:15:30PM +0530, Souptick Joarder wrote:
> Previouly drivers have their own way of mapping range of
> kernel pages/memory into user vma and this was done by
> invoking vm_insert_page() within a loop.
>
> As this pattern is common across different drivers, it can
> be generalized by creating a new function and use it across
> the drivers.
>
> vm_insert_range is the new API which will be used to map a
> range of kernel memory/pages to user vma.
>
> Signed-off-by: Souptick Joarder
> Reviewed-by: Matthew Wilcox
> ---
> include/linux/mm_types.h | 3 +++
> mm/memory.c | 28
> mm/nommu.c | 7 +++
> 3 files changed, 38 insertions(+)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 5ed8f62..15ae24f 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -523,6 +523,9 @@ extern void tlb_gather_mmu(struct mmu_gather *tlb, struct
> mm_struct *mm,
> extern void tlb_finish_mmu(struct mmu_gather *tlb,
> unsigned long start, unsigned long end);
>
> +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> + struct page **pages, unsigned long page_count);
> +
> static inline void init_tlb_flush_pending(struct mm_struct *mm)
> {
> atomic_set(&mm->tlb_flush_pending, 0);
> diff --git a/mm/memory.c b/mm/memory.c
> index 15c417e..da904ed 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1478,6 +1478,34 @@ static int insert_page(struct vm_area_struct *vma,
> unsigned long addr,
> }
>
> /**
> + * vm_insert_range - insert range of kernel pages into user vma
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pages: pointer to array of source kernel pages
> + * @page_count: no. of pages need to insert into user vma
> + *
> + * This allows drivers to insert range of kernel pages they've allocated
> + * into a user vma. This is a generic function which drivers can use
> + * rather than using their own way of mapping range of kernel pages into
> + * user vma.
Please add the return value and context descriptions.
> + */
> +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> + struct page **pages, unsigned long page_count)
> +{
> + unsigned long uaddr = addr;
> + int ret = 0, i;
> +
> + for (i = 0; i < page_count; i++) {
> + ret = vm_insert_page(vma, uaddr, pages[i]);
> + if (ret < 0)
> + return ret;
> + uaddr += PAGE_SIZE;
> + }
> +
> + return ret;
> +}
> +
> +/**
> * vm_insert_page - insert single page into user vma
> * @vma: user vma to map to
> * @addr: target user address of this page
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 749276b..d6ef5c7 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned
> long addr,
> }
> EXPORT_SYMBOL(vm_insert_page);
>
> +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> + struct page **pages, unsigned long page_count)
> +{
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(vm_insert_range);
> +
> /*
> * sys_brk() for the most part doesn't need the global kernel
> * lock, except when an application is doing something nasty
> --
> 1.9.1
>
--
Sincerely yours,
Mike.
Re: [PATCH 1/9] mm: Introduce new vm_insert_range API
On 11/16/18 12:15 AM, Souptick Joarder wrote:
> On Fri, Nov 16, 2018 at 12:11 PM Matthew Wilcox wrote:
>>
>> On Fri, Nov 16, 2018 at 11:00:30AM +0530, Souptick Joarder wrote:
>>> On Thu, Nov 15, 2018 at 11:44 PM Randy Dunlap wrote:
On 11/15/18 7:45 AM, Souptick Joarder wrote:
What is the opposite of vm_insert_range() or even of vm_insert_page()?
or is there no need for that?
>>>
>>> There is no opposite function of vm_insert_range() / vm_insert_page().
>>> My understanding is, in case of any error, mmap handlers will return the
>>> err to user process and user space will decide the next action. So next
>>> time when mmap handler is getting invoked it will map from the beginning.
>>> Correct me if I am wrong.
>>
>> The opposite function, I suppose, is unmap_region().
>>
s/no./number/
>>>
>>> I didn't get it ??
>>
>> This is a 'sed' expression. 's' is the 'substitute' command; the /
>> is a separator, 'no.' is what you wrote, and 'number' is what Randy
>> is recommending instead.
>
> Ok. Will change it in v2.
Thanks.
> + for (i = 0; i < page_count; i++) {
> + ret = vm_insert_page(vma, uaddr, pages[i]);
> + if (ret < 0)
> + return ret;
For a non-trivial value of page_count:
Is it a problem if vm_insert_page() succeeds for several pages
and then fails?
>>>
>>> No, it will be considered as total failure and mmap handler will return
>>> the err to user space.
>>
>> I think what Randy means is "What happens to the inserted pages?" and
>> the answer is that mmap_region() jumps to the 'unmap_and_free_vma'
>> label, which is an accurate name.
which says:
/* Undo any partial mapping done by a device driver. */
unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
and that is what I was missing. Thanks.
> Sorry for incorrect understanding of the question.
No problem.
--
~Randy
Re: [PATCH 08/15] ARM/arm64: sunxi: Move H3/H5 syscon label over to soc-specific nodes
Hi,
On Fri, Nov 16, 2018 at 05:47:50PM +0800, Chen-Yu Tsai wrote:
> On Fri, Nov 16, 2018 at 5:39 PM Maxime Ripard
> wrote:
> >
> > On Thu, Nov 15, 2018 at 03:50:06PM +0100, Paul Kocialkowski wrote:
> > > Now that we have specific nodes for the H3 and H5 system-controller
> > > that allow proper access to the EMAC clock configuration register,
> > > we no longer need a common dummy syscon node.
> > >
> > > Switch the syscon label over to each platform's dtsi file.
> > >
> > > Signed-off-by: Paul Kocialkowski
> > > ---
> > > arch/arm/boot/dts/sun8i-h3.dtsi | 2 +-
> > > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 6 --
> > > arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 2 +-
> > > 3 files changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi
> > > b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > index 7157d954fb8c..b337a9282783 100644
> > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > @@ -134,7 +134,7 @@
> > > };
> > >
> > > soc {
> > > - system-control@1c0 {
> > > + syscon: system-control@1c0 {
> > > compatible = "allwinner,sun8i-h3-system-control";
> > > reg = <0x01c0 0x1000>;
> > > #address-cells = <1>;
> > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > index 4b1530ebe427..9175ff0fb59a 100644
> > > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > @@ -152,12 +152,6 @@
> > > };
> > > };
> > >
> > > - syscon: syscon@1c0 {
> > > - compatible = "allwinner,sun8i-h3-system-controller",
> > > - "syscon";
> > > - reg = <0x01c0 0x1000>;
> > > - };
> > > -
> >
> > You're also dropping the syscon compatible there. But I'm not sure how
> > it could work with the H3 EMAC driver that would overwrite the
> > compatible already.
>
> I assume you are referring to the previous patch? The node names are not
> the same, hence the previous patch is adding another node for the system
> controller, and this patch removes the old one with the "syscon" compatible.
>
> We already patched the EMAC driver to support the new SRAM controller based
> regmap, so other than making people unhappy about having to update their
> DT, I don't think there would be any problems. This also means H3 in -next
> currently has _two_ syscon nodes.
Ah, indeed, I missed that. Pointing that out in the commit log could
be nice though.
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
I AWAIT YOUR SWIFT RESPONSE !!!
I am Vice Chairman of Hang Seng Bank, I have Important Matter to Discuss with you concerning my late client. Died without a NEXT OF KIN. Send me your private email for full details information. email me at E-Mail: draymondchie...@gmail.com Regards Mr.Fung
Re: Logitech QuickCam USB detected by Linux, but not user space applications
Dear Mauro, Thank you very much for the quick reply. On 11/15/18 12:38, Mauro Carvalho Chehab wrote: > Em Thu, 15 Nov 2018 11:42:32 +0100 Paul Menzel escreveu: >> I tried to get a Logitech QuickCam USB camera working, but unfortunately, it >> is >> not detected by user space (Cheese, MPlayer). > > Could you please try it with Camorama? > > https://github.com/alessio/camorama Thank you for the suggestion. At first, I only saw a black image, but changing the resolution made it work. See the status below. 1. does *not* work a) 160x120 b) 176x144 2. works a) 320x240 b) 352x288 >> It’s an old device, so it could be broken, but as it’s detected by the Linux >> kernel, I wanted to check with you first. >> >> Linux 4.18.10 from Debian Sid/unstable is used. >> >> ``` >> $ dmesg >> […] >> [ 2891.404361] usb 3-3: new full-speed USB device number 4 using ohci-pci >> [ 2891.626934] usb 3-3: New USB device found, idVendor=046d, idProduct=092e, >> bcdDevice= 0.00 >> [ 2891.626945] usb 3-3: New USB device strings: Mfr=1, Product=2, >> SerialNumber=0 >> [ 2891.626951] usb 3-3: Product: Camera >> [ 2891.626957] usb 3-3: Manufacturer: >> [ 2893.110249] calling media_devnode_init+0x0/0x1000 [media] @ 11704 >> [ 2893.110256] media: Linux media interface: v0.10 >> [ 2893.110329] initcall media_devnode_init+0x0/0x1000 [media] returned 0 >> after 56 usecs >> [ 2893.210078] calling videodev_init+0x0/0x79 [videodev] @ 11704 >> [ 2893.210084] videodev: Linux video capture interface: v2.00 >> [ 2893.210123] initcall videodev_init+0x0/0x79 [videodev] returned 0 after >> 21 usecs >> [ 2893.333140] calling gspca_init+0x0/0x1000 [gspca_main] @ 11704 >> [ 2893.333148] gspca_main: v2.14.0 registered >> [ 2893.333161] initcall gspca_init+0x0/0x1000 [gspca_main] returned 0 after >> 3 usecs >> [ 2893.370672] calling sd_driver_init+0x0/0x1000 [gspca_spca561] @ 11704 >> [ 2893.370751] gspca_main: spca561-2.14.0 probing 046d:092e >> [ 2893.482675] input: spca561 as >> /devices/pci:00/:00:12.0/usb3/3-3/input/input17 >> [ 2893.485415] usbcore: registered new interface driver spca561 >> [ 2893.485434] initcall sd_driver_init+0x0/0x1000 [gspca_spca561] returned 0 >> after 112054 usecs >> […] >> $ ls -l /dev/video* >> crw-rw+ 1 root video 81, 0 Nov 15 09:26 /dev/video0 >> >> $ mplayer tv:// -tv driver=v4l2:device=/dev/video0 >> MPlayer 1.3.0 (Debian), built with gcc-8 (C) 2000-2016 MPlayer Team >> do_connect: could not connect to socket >> connect: No such file or directory >> Failed to open LIRC support. You will not be able to use your remote control. >> >> Playing tv://. >> TV file format detected. >> Selected driver: v4l2 >> name: Video 4 Linux 2 input >> author: Martin Olschewski >> comment: first try, more to come ;-) >> v4l2: your device driver does not support VIDIOC_G_STD ioctl, VIDIOC_G_PARM >> was used instead. >> Selected device: Camera >> Capabilities: video capture read/write streaming >> supported norms: >> inputs: 0 = spca561; >> Current input: 0 >> Current format: unknown (0x31363553) > > The problem is likely here: mplayer is probably not using libv4l2. Without > that, it can't decode the spca561 specific output format. It is probably > due to some option used when mplayer was built. I’ll try to look more into that in the next weeks. > In the case of Cheese, it uses Gstreamer, with defaults to not use libv4l2 > either. On newest versions of it, there is an environment var that would > allow enabling it (I don't remember what var). Thank you for the details. I’ll test that next week. […] Kind regards, Paul smime.p7s Description: S/MIME Cryptographic Signature
Re: [PATCH v3] media: vivid: Improve timestamping
On 11/11/2018 11:39 PM, Gabriel Francisco Mandaji wrote:
> Simulate a more precise timestamp by calculating it based on the
> current framerate.
>
> Signed-off-by: Gabriel Francisco Mandaji
Something is still wrong: the frame period calculation is fine for
input 0 (webcam), but if I switch to another input (S-Video/TV/HDMI),
then it is totally wrong: for input 3 (HDMI) the fps seems to be off
by a factor of 1 (i.e. I see a framerate of over 6 where I
expected to see 60 Hz), and for inputs 2 and 3 I get 7000 fps for
PAL and 11 for NTSC.
v4l2-ctl -P (== VIDIOC_G_PARM) returns the right thing here.
This calculation is the problem:
u64 f_period = dev->timeperframe_vid_cap.numerator * 10 /
dev->timeperframe_vid_cap.denominator;
You need to cast dev->timeperframe_vid_cap.numerator to a u64
first otherwise the multiplication overflows the u32 if numerator is > 4.
Also note that you cannot divide 64 bit values: not all architectures
support that. Use do_div instead.
With that change it should work.
Regards,
Hans
> ---
> Changes in v2:
> - fix spelling
> - end of exposure is offset by 90% of the frame period
> - fix timestamp calculation for FIELD_ALTERNATE (untested)
> - timestamp is now calculated and set from vivid_thread_cap_tick()
> - capture vbi uses the same timestamp as non-vbi, but offset by 5%
> - timestamp stays consistent even if the FPS changes
> - tested with dropped frames
> Changes in v3:
> - fix calculating offset for 'End of Frame'
> - fix changing 'Timestamp Source' mid-capture
> - change order of operation when calculating the VBI's offset
> ---
> drivers/media/platform/vivid/vivid-core.h| 3 ++
> drivers/media/platform/vivid/vivid-kthread-cap.c | 47
> +---
> drivers/media/platform/vivid/vivid-vbi-cap.c | 4 --
> 3 files changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-core.h
> b/drivers/media/platform/vivid/vivid-core.h
> index 1891254..f7b2a0b 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -392,6 +392,9 @@ struct vivid_dev {
> /* thread for generating video capture stream */
> struct task_struct *kthread_vid_cap;
> unsigned long jiffies_vid_cap;
> + u64 cap_stream_start;
> + u64 cap_frame_period;
> + u64 cap_frame_eof_offset;
> u32 cap_seq_offset;
> u32 cap_seq_count;
> boolcap_seq_resync;
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c
> b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index eebfff2..f97cb6a 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -425,12 +425,6 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> is_loop = true;
>
> buf->vb.sequence = dev->vid_cap_seq_count;
> - /*
> - * Take the timestamp now if the timestamp source is set to
> - * "Start of Exposure".
> - */
> - if (dev->tstamp_src_is_soe)
> - buf->vb.vb2_buf.timestamp = ktime_get_ns();
> if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
> /*
>* 60 Hz standards start with the bottom field, 50 Hz standards
> @@ -554,14 +548,6 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> }
> }
> }
> -
> - /*
> - * If "End of Frame" is specified at the timestamp source, then take
> - * the timestamp now.
> - */
> - if (!dev->tstamp_src_is_soe)
> - buf->vb.vb2_buf.timestamp = ktime_get_ns();
> - buf->vb.vb2_buf.timestamp += dev->time_wrap_offset;
> }
>
> /*
> @@ -667,10 +653,25 @@ static void vivid_overlay(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> }
> }
>
> +static void vivid_cap_update_frame_period(struct vivid_dev *dev)
> +{
> + u64 f_period = dev->timeperframe_vid_cap.numerator * 10 /
> +dev->timeperframe_vid_cap.denominator;
> + if (dev->field_cap == V4L2_FIELD_ALTERNATE)
> + f_period /= 2;
> + /*
> + * If "End of Frame", then offset the exposure time by 0.9
> + * of the frame period.
> + */
> + dev->cap_frame_eof_offset = f_period * 9 / 10;
> + dev->cap_frame_period = f_period;
> +}
> +
> static void vivid_thread_vid_cap_tick(struct vivid_dev *dev, int
> dropped_bufs)
> {
> struct vivid_buffer *vid_cap_buf = NULL;
> struct vivid_buffer *vbi_cap_buf = NULL;
> + u64 f_time = 0;
>
> dprintk(dev, 1, "Video Capture Thread Tick\n");
>
> @@ -702,6 +703,11 @@ static void vivid_thre
I AWAIT YOUR SWIFT RESPONSE !!!
I am Vice Chairman of Hang Seng Bank, I have Important Matter to Discuss with you concerning my late client. Died without a NEXT OF KIN. Send me your private email for full details information. email me at E-Mail: draymondchie...@gmail.com Regards Mr.Fung
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
Hi Hans,
On Fri, Nov 16, 2018 at 02:31:01PM +0100, Hans Verkuil wrote:
> On 11/16/2018 02:26 PM, Sakari Ailus wrote:
> > Hi Marco, Michael,
> >
> > On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
> >> From: Michael Grzeschik
> >>
> >> This patch implements the framerate selection using the skipping and
> >> readout power-modi features. The power-modi cut the framerate by half
> >> and each context has an independent selection bit. The same applies to
> >> the 2x skipping feature.
> >>
> >> Signed-off-by: Michael Grzeschik
> >> Signed-off-by: Marco Felsch
> >>
> >> ---
> >> Changelog
> >>
> >> v2:
> >> - fix updating read mode register, use mt9m111_reg_mask() to update the
> >> relevant bits only. For this purpose add reg_mask field to
> >> struct mt9m111_mode_info.
> >>
> >> drivers/media/i2c/mt9m111.c | 163
> >> 1 file changed, 163 insertions(+)
> >>
> >> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>
>
>
> >> +static const struct mt9m111_mode_info *
> >> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> >> +unsigned int width, unsigned int height)
> >> +{
> >> + const struct mt9m111_mode_info *mode;
> >> + struct v4l2_rect *sensor_rect = &mt9m111->rect;
> >> + unsigned int gap, gap_best = (unsigned int) -1;
> >> + int i, best_gap_idx = 1;
> >> +
> >> + /* find best matched fps */
> >> + for (i = 0; i < MT9M111_NUM_MODES; i++) {
> >> + unsigned int fps = mt9m111_mode_data[i].max_fps;
> >> +
> >> + gap = abs(fps - req_fps);
> >> + if (gap < gap_best) {
> >> + best_gap_idx = i;
> >> + gap_best = gap;
> >> + }
> >
> > Could you use v4l2_find_nearest_size() instead?
> >
> > Also see below...
> >
> >> + }
> >> +
> >> + /*
> >> + * Use context a/b default timing values instead of calculate blanking
> >> + * timing values.
> >> + */
> >> + mode = &mt9m111_mode_data[best_gap_idx];
> >> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? &context_a :
> >> + &context_b;
> >> +
> >> + /*
> >> + * Check if current settings support the fps because fps selection is
> >> + * based on the row/col skipping mechanism which has some restriction.
> >> + */
> >> + if (sensor_rect->width != mode->sensor_w ||
> >> + sensor_rect->height != mode->sensor_h ||
> >> + width > mode->max_image_w ||
> >> + height > mode->max_image_h) {
> >> + /* reset sensor window size */
> >> + mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
> >> + mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
> >> + mt9m111->rect.width = mode->sensor_w;
> >> + mt9m111->rect.height = mode->sensor_h;
> >> +
> >> + /* reset image size */
> >> + mt9m111->width = mode->max_image_w;
> >> + mt9m111->height = mode->max_image_h;
> >> +
> >> + dev_warn(mt9m111->subdev.dev,
> >> + "Warning: update image size %dx%d[%dx%d] ->
> >> %dx%d[%dx%d]\n",
> >> + sensor_rect->width, sensor_rect->height, width, height,
> >> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
> >> + mode->max_image_h);
> >
> > I wouldn't expect requesting a particular frame rate to change the sensor
> > format. The other way around is definitely fine though.
> >
> > Cc Hans.
>
> I agree with Sakari. Changing the framerate should never change the format.
> When you enumerate framerates those framerates are the allowed framerates
> for the mediabus format and the resolution. So changing the framerate should
> never modify the format or resolution. Instead, the framerate should be
> mapped to a framerate that is valid for the format/resolution combo.
I don't think this is actually documented, at least not for the sub-device
API. I can send a patch.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 4/6] media: mt9m111: allow to setup pixclk polarity
Hi Marco, Enrico,
On Mon, Oct 29, 2018 at 07:24:08PM +0100, Marco Felsch wrote:
> From: Enrico Scholz
>
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
>
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.
>
> Signed-off-by: Enrico Scholz
> (m.grzesc...@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik
> (m.fel...@pengutronix.de: use fwnode helpers)
> (m.fel...@pengutronix.de: mv fw parsing into own function)
> (m.fel...@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch
>
> ---
> Changelog:
>
> v2:
> - make use of fwnode_*() to drop OF dependency and ifdef's
> - mt9m111_g_mbus_config: fix pclk_sample logic which I made due the
> conversion from Enrico's patch.
>
> drivers/media/i2c/mt9m111.c | 46 -
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index e9879e111f58..112eaa5ba917 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -15,12 +15,14 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> #include
> #include
> +#include
>
> /*
> * MT9M111, MT9M112 and MT9M131:
> @@ -239,6 +241,8 @@ struct mt9m111 {
> const struct mt9m111_datafmt *fmt;
> int lastpage; /* PageMap cache value */
> bool is_streaming;
> + /* user point of view - 0: falling 1: rising edge */
> + unsigned int pclk_sample:1;
You could use a bool. Up to you.
> #ifdef CONFIG_MEDIA_CONTROLLER
> struct media_pad pad;
> #endif
> @@ -591,6 +595,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> return -EINVAL;
> }
>
> + /* receiver samples on falling edge, chip-hw default is rising */
> + if (mt9m111->pclk_sample == 0)
> + mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> +
> ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> data_outfmt2, mask_outfmt2);
> if (!ret)
> @@ -1051,9 +1059,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd,
> int enable)
> static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> struct v4l2_mbus_config *cfg)
> {
> - cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> + struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> + cfg->flags = V4L2_MBUS_MASTER |
> V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> + cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> + V4L2_MBUS_PCLK_SAMPLE_FALLING;
> +
> cfg->type = V4L2_MBUS_PARALLEL;
>
> return 0;
> @@ -1123,6 +1137,32 @@ static int mt9m111_video_probe(struct i2c_client
> *client)
> return ret;
> }
>
> +static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111
> *mt9m111)
> +{
> + struct v4l2_fwnode_endpoint *bus_cfg;
> + struct fwnode_handle *np;
> + int ret = 0;
> +
> + np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> + if (!np)
> + return -EINVAL;
> +
> + bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> + if (IS_ERR(bus_cfg)) {
> + ret = PTR_ERR(bus_cfg);
> + goto out_put_fw;
> + }
> +
> + mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> + V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> + v4l2_fwnode_endpoint_free(bus_cfg);
> +
> +out_put_fw:
> + fwnode_handle_put(np);
> + return ret;
> +}
> +
> static int mt9m111_probe(struct i2c_client *client,
>const struct i2c_device_id *did)
> {
> @@ -1147,6 +1187,10 @@ static int mt9m111_probe(struct i2c_client *client,
> /* Default HIGHPOWER context */
> mt9m111->ctx = &context_b;
>
> + ret = mt9m111_probe_fw(client, mt9m111);
> + if (ret)
> + return ret;
Can you do this before v4l2_clk_get()? That'll go anyway, but for now,
you'd need extra error handling for it.
> +
> v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
> mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
Please also put this patch after the DT binding changes.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
On 11/16/2018 02:26 PM, Sakari Ailus wrote:
> Hi Marco, Michael,
>
> On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
>> From: Michael Grzeschik
>>
>> This patch implements the framerate selection using the skipping and
>> readout power-modi features. The power-modi cut the framerate by half
>> and each context has an independent selection bit. The same applies to
>> the 2x skipping feature.
>>
>> Signed-off-by: Michael Grzeschik
>> Signed-off-by: Marco Felsch
>>
>> ---
>> Changelog
>>
>> v2:
>> - fix updating read mode register, use mt9m111_reg_mask() to update the
>> relevant bits only. For this purpose add reg_mask field to
>> struct mt9m111_mode_info.
>>
>> drivers/media/i2c/mt9m111.c | 163
>> 1 file changed, 163 insertions(+)
>>
>> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
>> +static const struct mt9m111_mode_info *
>> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
>> + unsigned int width, unsigned int height)
>> +{
>> +const struct mt9m111_mode_info *mode;
>> +struct v4l2_rect *sensor_rect = &mt9m111->rect;
>> +unsigned int gap, gap_best = (unsigned int) -1;
>> +int i, best_gap_idx = 1;
>> +
>> +/* find best matched fps */
>> +for (i = 0; i < MT9M111_NUM_MODES; i++) {
>> +unsigned int fps = mt9m111_mode_data[i].max_fps;
>> +
>> +gap = abs(fps - req_fps);
>> +if (gap < gap_best) {
>> +best_gap_idx = i;
>> +gap_best = gap;
>> +}
>
> Could you use v4l2_find_nearest_size() instead?
>
> Also see below...
>
>> +}
>> +
>> +/*
>> + * Use context a/b default timing values instead of calculate blanking
>> + * timing values.
>> + */
>> +mode = &mt9m111_mode_data[best_gap_idx];
>> +mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? &context_a :
>> +&context_b;
>> +
>> +/*
>> + * Check if current settings support the fps because fps selection is
>> + * based on the row/col skipping mechanism which has some restriction.
>> + */
>> +if (sensor_rect->width != mode->sensor_w ||
>> +sensor_rect->height != mode->sensor_h ||
>> +width > mode->max_image_w ||
>> +height > mode->max_image_h) {
>> +/* reset sensor window size */
>> +mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
>> +mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
>> +mt9m111->rect.width = mode->sensor_w;
>> +mt9m111->rect.height = mode->sensor_h;
>> +
>> +/* reset image size */
>> +mt9m111->width = mode->max_image_w;
>> +mt9m111->height = mode->max_image_h;
>> +
>> +dev_warn(mt9m111->subdev.dev,
>> + "Warning: update image size %dx%d[%dx%d] ->
>> %dx%d[%dx%d]\n",
>> + sensor_rect->width, sensor_rect->height, width, height,
>> + mode->sensor_w, mode->sensor_h, mode->max_image_w,
>> + mode->max_image_h);
>
> I wouldn't expect requesting a particular frame rate to change the sensor
> format. The other way around is definitely fine though.
>
> Cc Hans.
I agree with Sakari. Changing the framerate should never change the format.
When you enumerate framerates those framerates are the allowed framerates
for the mediabus format and the resolution. So changing the framerate should
never modify the format or resolution. Instead, the framerate should be
mapped to a framerate that is valid for the format/resolution combo.
Regards,
Hans
Re: [PATCH v2 3/6] media: mt9m111: add support to select formats and fps for {Q,SXGA}
Hi Marco, Michael,
On Mon, Oct 29, 2018 at 07:24:07PM +0100, Marco Felsch wrote:
> From: Michael Grzeschik
>
> This patch implements the framerate selection using the skipping and
> readout power-modi features. The power-modi cut the framerate by half
> and each context has an independent selection bit. The same applies to
> the 2x skipping feature.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marco Felsch
>
> ---
> Changelog
>
> v2:
> - fix updating read mode register, use mt9m111_reg_mask() to update the
> relevant bits only. For this purpose add reg_mask field to
> struct mt9m111_mode_info.
>
> drivers/media/i2c/mt9m111.c | 163
> 1 file changed, 163 insertions(+)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 1f8789fe28af..e9879e111f58 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -126,6 +126,8 @@
> #define MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN (1 << 1)
> #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B (1 << 0)
> #define MT9M111_TPG_SEL_MASK GENMASK(2, 0)
> +#define MT9M111_RM_PWR_MASK BIT(10)
> +#define MT9M111_RM_SKIP2_MASKGENMASK(3, 2)
>
> /*
> * Camera control register addresses (0x200..0x2ff not implemented)
> @@ -204,6 +206,23 @@ static const struct mt9m111_datafmt
> mt9m111_colour_fmts[] = {
> {MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
> };
>
> +enum mt9m111_mode_id {
> + MT9M111_MODE_SXGA_8FPS,
> + MT9M111_MODE_SXGA_15FPS,
> + MT9M111_MODE_QSXGA_30FPS,
> + MT9M111_NUM_MODES,
> +};
> +
> +struct mt9m111_mode_info {
> + unsigned int sensor_w;
> + unsigned int sensor_h;
> + unsigned int max_image_w;
> + unsigned int max_image_h;
> + unsigned int max_fps;
> + unsigned int reg_val;
> + unsigned int reg_mask;
> +};
> +
> struct mt9m111 {
> struct v4l2_subdev subdev;
> struct v4l2_ctrl_handler hdl;
> @@ -213,6 +232,8 @@ struct mt9m111 {
> struct v4l2_clk *clk;
> unsigned int width; /* output */
> unsigned int height;/* sizes */
> + struct v4l2_fract frame_interval;
> + const struct mt9m111_mode_info *current_mode;
> struct mutex power_lock; /* lock to protect power_count */
> int power_count;
> const struct mt9m111_datafmt *fmt;
> @@ -223,6 +244,37 @@ struct mt9m111 {
> #endif
> };
>
> +static const struct mt9m111_mode_info mt9m111_mode_data[MT9M111_NUM_MODES] =
> {
> + [MT9M111_MODE_SXGA_8FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 1280,
> + .max_image_h = 1024,
> + .max_fps = 8,
> + .reg_val = MT9M111_RM_LOW_POWER_RD,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> + [MT9M111_MODE_SXGA_15FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 1280,
> + .max_image_h = 1024,
> + .max_fps = 15,
> + .reg_val = MT9M111_RM_FULL_POWER_RD,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> + [MT9M111_MODE_QSXGA_30FPS] = {
> + .sensor_w = 1280,
> + .sensor_h = 1024,
> + .max_image_w = 640,
> + .max_image_h = 512,
> + .max_fps = 30,
> + .reg_val = MT9M111_RM_LOW_POWER_RD | MT9M111_RM_COL_SKIP_2X |
> +MT9M111_RM_ROW_SKIP_2X,
> + .reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
> + },
> +};
> +
> /* Find a data format by a pixel code */
> static const struct mt9m111_datafmt *mt9m111_find_datafmt(struct mt9m111
> *mt9m111,
> u32 code)
> @@ -615,6 +667,62 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> return ret;
> }
>
> +static const struct mt9m111_mode_info *
> +mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> + unsigned int width, unsigned int height)
> +{
> + const struct mt9m111_mode_info *mode;
> + struct v4l2_rect *sensor_rect = &mt9m111->rect;
> + unsigned int gap, gap_best = (unsigned int) -1;
> + int i, best_gap_idx = 1;
> +
> + /* find best matched fps */
> + for (i = 0; i < MT9M111_NUM_MODES; i++) {
> + unsigned int fps = mt9m111_mode_data[i].max_fps;
> +
> + gap = abs(fps - req_fps);
> + if (gap < gap_best) {
> + best_gap_idx = i;
> + gap_best = gap;
> + }
Could you use v4l2_find_nearest_size() instead?
Also see below...
> + }
> +
> + /*
> + * Use context a/b default timing values instead of calculate blanking
> + * timing values.
> + */
> + mode = &mt9m111_mode_data[best_gap_idx];
> + mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? &contex
Re: [PATCH 4/4] media: ov5640: Add additional media bus formats
Hi Sam,
On Mon, Oct 08, 2018 at 11:48:02PM -0700, Sam Bobrowicz wrote:
> Add support for 1X16 yuv media bus formats (v4l2_mbus_framefmt).
> These formats are equivalent to the 2X8 formats that are already
> supported, both of which accurately describe the data present on
> the CSI2 interface. This change will increase compatibility with
> CSI2 RX drivers that only advertise support for the 1X16 formats.
>
> Signed-off-by: Sam Bobrowicz
> ---
> drivers/media/i2c/ov5640.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index a50d884..ca9de56 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -125,6 +125,8 @@ static const struct ov5640_pixfmt ov5640_formats[] = {
> { MEDIA_BUS_FMT_JPEG_1X8, V4L2_COLORSPACE_JPEG, },
> { MEDIA_BUS_FMT_UYVY8_2X8, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_YUYV8_2X8, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_UYVY8_1X16, V4L2_COLORSPACE_SRGB, },
> + { MEDIA_BUS_FMT_YUYV8_1X16, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_LE, V4L2_COLORSPACE_SRGB, },
> { MEDIA_BUS_FMT_RGB565_2X8_BE, V4L2_COLORSPACE_SRGB, },
> };
Can you make these selectable only for the CSI-2 bus? I understand the
driver also supports the parallel interface.
> @@ -2069,10 +2071,12 @@ static int ov5640_set_framefmt(struct ov5640_dev
> *sensor,
>
> switch (format->code) {
> case MEDIA_BUS_FMT_UYVY8_2X8:
> + case MEDIA_BUS_FMT_UYVY8_1X16:
> /* YUV422, UYVY */
> val = 0x3f;
> break;
> case MEDIA_BUS_FMT_YUYV8_2X8:
> + case MEDIA_BUS_FMT_YUYV8_1X16:
> /* YUV422, YUYV */
> val = 0x30;
> break;
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCHv2 0/9] vb2/cedrus: add tag support
On 11/16/2018 10:00 AM, Tomasz Figa wrote: > Hi Hans, > > On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil wrote: >> >> From: Hans Verkuil >> >> As was discussed here (among other places): >> >> https://lkml.org/lkml/2018/10/19/440 >> >> using capture queue buffer indices to refer to reference frames is >> not a good idea. A better idea is to use a 'tag' where the >> application can assign a u64 tag to an output buffer, which is then >> copied to the capture buffer(s) derived from the output buffer. >> > > Thanks for the patches. Please see my comments below. > >> A u64 is chosen since this allows userspace to also use pointers to >> internal structures as 'tag'. >> > > I think this is not true anymore in this version. Ah, forgot to update the commit message. > >> The first three patches add core tag support, the next patch document >> the tag support, then a new helper function is added to v4l2-mem2mem.c >> to easily copy data from a source to a destination buffer that drivers >> can use. >> >> Next a new supports_tags vb2_queue flag is added to indicate that >> the driver supports tags. Ideally this should not be necessary, but >> that would require that all m2m drivers are converted to using the >> new helper function introduced in the previous patch. That takes more >> time then I have now since we want to get this in for 4.20. >> >> Finally the vim2m, vicodec and cedrus drivers are converted to support >> tags. >> >> I also removed the 'pad' fields from the mpeg2 control structs (it >> should never been added in the first place) and aligned the structs >> to a u32 boundary (u64 for the tag values). > > u32 in this version > >> >> Note that this might change further (Paul suggested using bitfields). >> >> Also note that the cedrus code doesn't set the sequence counter, that's >> something that should still be added before this driver can be moved >> out of staging. >> >> Note: if no buffer is found for a certain tag, then the dma address >> is just set to 0. That happened before as well with invalid buffer >> indices. This should be checked in the driver! > > Note that DMA address 0 may as well be a valid address. Should we have > another way of signaling that? See this patch: https://patchwork.linuxtv.org/patch/52975/ The memory of the reference buffer should be checked and the refcount incremented in the request validate function. Once that's done the dma address will be guaranteed to be OK. This is missing functionality that is important to add. > >> >> The previous RFC series was tested successfully with the cedrus driver. >> >> Regards, >> >> Hans >> >> Changes since v1: >> >> - changed to a u32 tag. Using a 64 bit tag was overly complicated due >> to the bad layout of the v4l2_buffer struct, and there is no real >> need for it by applications. > > I hope these won't become famous last words. :) For Chromium it should > be okay indeed. The only 'feature' that is now missing is the ability to store pointers as the tag. So worst case you need to do an extra lookup in the application. > > Since we've been thinking about a redesign of the buffer struct, > perhaps we can live with u32 for now and we can design the new struct > to handle u64 nicely? It could be done, but I'm not sure if it is a good idea to have different behavior between v4l2_buffer and v4l2_ext_buffer. Something to discuss at that time. Regards, Hans > > Best regards, > Tomasz >
Re: [PATCH v5 0/2] media: platform: Add Aspeed Video Engine Driver
On 11/15/2018 10:20 AM, Hans Verkuil wrote: > On 11/12/2018 10:00 PM, Eddie James wrote: >> From: Eddie James >> >> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs >> can capture and compress video data from digital or analog sources. With >> the Aspeed chip acting as a service processor, the Video Engine can >> capture the host processor graphics output. >> >> This series adds a V4L2 driver for the VE, providing the usual V4L2 streaming >> interface by way of videobuf2. Each frame, the driver triggers the hardware >> to >> capture the host graphics output and compress it to JPEG format. > > I reviewed the driver, and there is still confusion about active timings and > detected timings. You are really close, but it is still not right. > > The timings returned by G_DV_TIMINGS should never change, unless by calling > S_DV_TIMINGS. The format returned by G_FMT should never change, unless by > calling S_DV_TIMINGS (which implicitly changes the format) or S_FMT. > > Timings changes from the video or calling QUERY_DV_TIMINGS should have NO > effect on the timings/format returned by G_DV_TIMINGS/G_FMT. > > Obviously, if the video timings change, then streaming will halt and an event > is issued so userspace can take action. > >> >> v4l-utils HEAD dd3ff81f58c4e1e6f33765dc61ad33c48ae6bb07 >> >> v4l2-compliance SHA: not available, 32 bits > > > >> Control ioctls (Input 0): >> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK >> test VIDIOC_QUERYCTRL: OK >> test VIDIOC_G/S_CTRL: OK >> test VIDIOC_G/S/TRY_EXT_CTRLS: OK >> fail: v4l2-test-controls.cpp(823): failed to find event for >> control 'Chroma Subsampling' > > That's weird. It's not clear to me why this fails. Ah, this is caused by a bug in our tree. It's fixed by this patch: https://patchwork.linuxtv.org/patch/52790/ Regards, Hans > >> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL >> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) >> Standard Controls: 3 Private Controls: 0 >> >> Format ioctls (Input 0): >> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK >> test VIDIOC_G/S_PARM: OK >> test VIDIOC_G_FBUF: OK (Not Supported) >> test VIDIOC_G_FMT: OK >> test VIDIOC_TRY_FMT: OK >> test VIDIOC_S_FMT: OK >> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) >> test Cropping: OK (Not Supported) >> test Composing: OK (Not Supported) >> test Scaling: OK (Not Supported) >> >> Codec ioctls (Input 0): >> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) >> test VIDIOC_G_ENC_INDEX: OK (Not Supported) >> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) >> >> Buffer ioctls (Input 0): >> fail: v4l2-test-buffers.cpp(422): dmabuf_valid ^ !!(caps & >> V4L2_BUF_CAP_SUPPORTS_DMABUF) > > I updated v4l2-compliance, as this test was a bit too strict. > >> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL >> test VIDIOC_EXPBUF: OK (Not Supported) >> >> Test input 0: >> >> Streaming ioctls: >> test read/write: OK >> test blocking wait: OK >> test MMAP: OK >> test USERPTR: OK (Not Supported) >> fail: v4l2-test-buffers.cpp(1102): exp_q.reqbufs(expbuf_node, >> q.g_buffers()) >> fail: v4l2-test-buffers.cpp(1192): setupDmaBuf(expbuf_node, >> node, q, exp_q) > > You need to add: > > .vidioc_expbuf = vb2_ioctl_expbuf, > > to aspeed_video_ioctl_ops. > >> test DMABUF: FAIL >> >> Total: 48, Succeeded: 45, Failed: 3, Warnings: 0 >> >> The apparent rate of change to the v4l2/vb2 API makes it difficult to pass >> these tests. None of the failing tests even ran last time I submitted my >> series. And V4L2_BUF_CAP_SUPPORTS_DMABUF is undefined in 4.19. > > When developing new drivers you should use the master branch of > https://git.linuxtv.org/media_tree.git/ > > The v4l-utils repo is kept in sync with the latest code from that master > branch. > > Regards, > > Hans > >> >> Changes since v4: >> - Set real min and max resolution in enum_dv_timings >> - Add check for vb2_is_busy before settings the timings >> - Set max frame rate to the actual max rather than max + 1 >> - Correct the input status to 0. >> - Rework resolution change to only set the relevant h/w regs during startup >> or >>when set_timings is called. >> >> Changes since v3: >> - Switch update reg function to use u32 clear rather than unsigned long mask >> - Add timing information from host VGA signal >> - Fix binding documentation mispelling >> - Fix upper case hex values >> - Add wait_prepare and wait_finish >> - Set buffer state to queued (rather than error) if streaming fails to start >> - Switch engine busy print statement to error >> - Removed a couple unecessary type assignments in v4l2 ioctls >> - Added query_dv_timings, fixed get_dv_timings >> - Corrected source change event to fire if and only if size actually changes >> - Locke
Re: [PATCH] media: cedrus: Remove global IRQ spin lock from the driver
On Thu, Nov 15, 2018 at 03:39:55PM +0100, Paul Kocialkowski wrote: > We initially introduced a spin lock to ensure that the VPU registers > are not accessed concurrently between our setup function and IRQ > handler. Because the V4L2 M2M API only allows one job to run at a time > and our jobs are completed following the IRQ handler, there is actually > no chance of an interrupt happening while programming the VPU registers. That's not entirely true. There's no chance that the interrupt signaling the end of the frame decoding can happen. However, spurious interrupts can happen at any point in time as soon as you have the interrupts enabled. > In addition, holding a spin lock is problematic when doing more than > simply configuring the registers in the setup operation. H.265 support > currently involves a CMA allocation in that step, which is not > compatible with an atomic context. That's not really true either. Any allocation can be done with GFP_ATOMIC or GFP_NOWAIT, and be compatible with an atomic context. Whether it's something we want is a different story :) And since the h265 code isn't upstream, I'm not really sure it's relevant to mention it here. > As a result, remove the global IRQ spin lock. > > Signed-off-by: Paul Kocialkowski Acked-by: Maxime Ripard Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com signature.asc Description: PGP signature
Re: [PATCH 01/15] ARM: dts: sun8i-a33: Remove heading 0 in video-codec unit address
Hi, Le jeudi 15 novembre 2018 à 23:50 +0800, Chen-Yu Tsai a écrit : > On Thu, Nov 15, 2018 at 10:50 PM Paul Kocialkowski > wrote: > > This cosmetic change removes the heading 0 in the video-codec unit > > address, as it's done for other nodes. > > > > Signed-off-by: Paul Kocialkowski > > Nit: I'd prefer the subject prefix format be ": : ... ", > or "sun8i: a33:" in this case. This format seems to be used more often > than your alternative format. > > I can fix it up when applying. Understood, I will make sure to follow this convention next time. Cheers, Paul > Acked-by: Chen-Yu Tsai > > ChenYu
Re: [PATCH 08/15] ARM/arm64: sunxi: Move H3/H5 syscon label over to soc-specific nodes
Hi,
Le vendredi 16 novembre 2018 à 17:47 +0800, Chen-Yu Tsai a écrit :
> On Fri, Nov 16, 2018 at 5:39 PM Maxime Ripard
> wrote:
> > On Thu, Nov 15, 2018 at 03:50:06PM +0100, Paul Kocialkowski wrote:
> > > Now that we have specific nodes for the H3 and H5 system-controller
> > > that allow proper access to the EMAC clock configuration register,
> > > we no longer need a common dummy syscon node.
> > >
> > > Switch the syscon label over to each platform's dtsi file.
> > >
> > > Signed-off-by: Paul Kocialkowski
> > > ---
> > > arch/arm/boot/dts/sun8i-h3.dtsi | 2 +-
> > > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 6 --
> > > arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 2 +-
> > > 3 files changed, 2 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi
> > > b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > index 7157d954fb8c..b337a9282783 100644
> > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > @@ -134,7 +134,7 @@
> > > };
> > >
> > > soc {
> > > - system-control@1c0 {
> > > + syscon: system-control@1c0 {
> > > compatible = "allwinner,sun8i-h3-system-control";
> > > reg = <0x01c0 0x1000>;
> > > #address-cells = <1>;
> > > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > index 4b1530ebe427..9175ff0fb59a 100644
> > > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > > @@ -152,12 +152,6 @@
> > > };
> > > };
> > >
> > > - syscon: syscon@1c0 {
> > > - compatible = "allwinner,sun8i-h3-system-controller",
> > > - "syscon";
> > > - reg = <0x01c0 0x1000>;
> > > - };
> > > -
> >
> > You're also dropping the syscon compatible there. But I'm not sure how
> > it could work with the H3 EMAC driver that would overwrite the
> > compatible already.
>
> I assume you are referring to the previous patch? The node names are not
> the same, hence the previous patch is adding another node for the system
> controller, and this patch removes the old one with the "syscon" compatible.
>
> We already patched the EMAC driver to support the new SRAM controller based
> regmap, so other than making people unhappy about having to update their
> DT, I don't think there would be any problems. This also means H3 in -next
> currently has _two_ syscon nodes.
Yes, the point is indeed to only have a single node per platform (in
the platform dtsi) instead of two (one in the common h3-h5 dtsi and one
in the platform dtsi).
I guess updating the dt is not even a hard requirement after this
series: things will keep working with the dummy syscon node for giving
the EMAC driver access to the syscon registers.
Cheers,
Paul
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On 11/16/2018 09:45 AM, Tomasz Figa wrote:
> On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil wrote:
>>
>> On 11/16/2018 09:34 AM, Tomasz Figa wrote:
>>> Hi Hans,
>>>
>>> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
Calling the stop_streaming op can release the core serialization lock
pointed to by vb2_queue->lock if it has to wait for buffers to finish.
An example of that behavior is the vivid driver.
>>>
>>> Why would vb2_queue->lock have to be released to wait for buffer to
>>> finish? The drivers I worked with never had to do anything like that.
>>
>> Actually, they all do. It's done through the wait_prepare/finish callbacks
>> and by setting those to vb2_ops_wait_prepare/finish.
>>
>> If you don't, then while one thread is waiting for a buffer to arrive,
>> another thread cannot queue a new buffer since it will be serialized by
>> queue->lock.
>>
>> v4l2-compliance even tests for this.
>
> Why would you need the userspace to queue more buffers when you're
> stopping the queue?
Ah, I misunderstood your question. Your question was: why should stop_streaming
have to release the lock when it waits for buffers to finish.
In this case (vivid) the thread generating the image takes the main lock, which
is the same as queue->lock. So stop_streaming (which is called with the same
lock taken) has to unlock it, stop the thread, then retake it.
I thought this would be more common, but after analyzing other usages of kthread
it appears to be vivid specific. So I agree that it is better to fix vivid
instead of messing about with vb2.
Regards,
Hans
>
>>
>>>
However, if userspace dup()ped the video device filehandle, then it is
possible to stop streaming on one filehandle and call read/write or
VIDIOC_QBUF from the other.
>>>
>>> How about other ioctls? I can imagine at least STREAMON could be
>>> called at the same time too, but not sure if it would have any side
>>> effects.
>>
>> STREAMON would return an error since q->streaming is still set while
>> in the stop_streaming callback.
>>
>> So that combination is safe.
>>
>
> Okay, thanks. I'm still slightly worried that this approach with a
> flag makes it possible to miss some non-trivial cases, though...
>
>> Regards,
>>
>> Hans
>>
>>>
>>> Best regards,
>>> Tomasz
>>>
This is fixed by setting a flag whenever stop_streaming is called and
checking the flag where needed so we can return -EBUSY.
Signed-off-by: Hans Verkuil
Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
---
drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
include/media/videobuf2-core.h | 1 +
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 138223af701f..560577321fe7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
index, void *pb,
dprintk(1, "fatal error occurred on queue\n");
return -EIO;
}
+ if (q->in_stop_streaming) {
+ dprintk(1, "stop_streaming is called\n");
+ return -EBUSY;
+ }
vb = q->bufs[index];
@@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
* Tell driver to stop all transactions and release all queued
* buffers.
*/
- if (q->start_streaming_called)
+ if (q->start_streaming_called) {
+ q->in_stop_streaming = 1;
call_void_qop(q, stop_streaming, q);
+ q->in_stop_streaming = 0;
+ }
/*
* If you see this warning, then the driver isn't cleaning up
properly
@@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
*q, char __user *data, size_
return -EBUSY;
}
+ if (q->in_stop_streaming) {
+ dprintk(3, "stop_streaming is called\n");
+ return -EBUSY;
+ }
+
/*
* Initialize emulator on first call.
*/
diff --git a/include/media/videobuf2-core.h
b/include/media/videobuf2-core.h
index 613f22910174..5a3d3ada5940 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -585,6 +585,7 @@ struct vb2_queue {
unsigned interror:1;
unsigned intwaiting_for_buffers:1;
unsigned intwaiting_in_dqbuf:1;
+ unsigned intin_stop_streaming:1;
unsigned
Re: [PATCH 08/15] ARM/arm64: sunxi: Move H3/H5 syscon label over to soc-specific nodes
On Fri, Nov 16, 2018 at 5:39 PM Maxime Ripard wrote:
>
> On Thu, Nov 15, 2018 at 03:50:06PM +0100, Paul Kocialkowski wrote:
> > Now that we have specific nodes for the H3 and H5 system-controller
> > that allow proper access to the EMAC clock configuration register,
> > we no longer need a common dummy syscon node.
> >
> > Switch the syscon label over to each platform's dtsi file.
> >
> > Signed-off-by: Paul Kocialkowski
> > ---
> > arch/arm/boot/dts/sun8i-h3.dtsi | 2 +-
> > arch/arm/boot/dts/sunxi-h3-h5.dtsi | 6 --
> > arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 2 +-
> > 3 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi
> > b/arch/arm/boot/dts/sun8i-h3.dtsi
> > index 7157d954fb8c..b337a9282783 100644
> > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > @@ -134,7 +134,7 @@
> > };
> >
> > soc {
> > - system-control@1c0 {
> > + syscon: system-control@1c0 {
> > compatible = "allwinner,sun8i-h3-system-control";
> > reg = <0x01c0 0x1000>;
> > #address-cells = <1>;
> > diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > index 4b1530ebe427..9175ff0fb59a 100644
> > --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> > @@ -152,12 +152,6 @@
> > };
> > };
> >
> > - syscon: syscon@1c0 {
> > - compatible = "allwinner,sun8i-h3-system-controller",
> > - "syscon";
> > - reg = <0x01c0 0x1000>;
> > - };
> > -
>
> You're also dropping the syscon compatible there. But I'm not sure how
> it could work with the H3 EMAC driver that would overwrite the
> compatible already.
I assume you are referring to the previous patch? The node names are not
the same, hence the previous patch is adding another node for the system
controller, and this patch removes the old one with the "syscon" compatible.
We already patched the EMAC driver to support the new SRAM controller based
regmap, so other than making people unhappy about having to update their
DT, I don't think there would be any problems. This also means H3 in -next
currently has _two_ syscon nodes.
ChenYu
[PATCH v4l-utils] keytable: match every entry in rc_maps.cfg, not just the first
Signed-off-by: Sean Young
---
utils/keytable/keytable.c | 71 ---
1 file changed, 36 insertions(+), 35 deletions(-)
diff --git a/utils/keytable/keytable.c b/utils/keytable/keytable.c
index e15440de..df9cfc49 100644
--- a/utils/keytable/keytable.c
+++ b/utils/keytable/keytable.c
@@ -634,14 +634,13 @@ static error_t parse_keyfile(char *fname, char **table)
return parse_plain_keyfile(fname, table);
}
-struct cfgfile *nextcfg = &cfg;
-
static error_t parse_cfgfile(char *fname)
{
FILE *fin;
int line = 0;
char s[2048];
char *driver, *table, *filename;
+ struct cfgfile *nextcfg = &cfg;
if (debug)
fprintf(stderr, _("Parsing %s config file\n"), fname);
@@ -2142,55 +2141,57 @@ int main(int argc, char *argv[])
struct cfgfile *cur;
char *fname, *name;
int rc;
+ int matches = 0;
for (cur = &cfg; cur->next; cur = cur->next) {
if ((!rc_dev.drv_name || strcasecmp(cur->driver,
rc_dev.drv_name)) && strcasecmp(cur->driver, "*"))
continue;
if ((!rc_dev.keytable_name || strcasecmp(cur->table,
rc_dev.keytable_name)) && strcasecmp(cur->table, "*"))
continue;
- break;
- }
- if (!cur->next) {
if (debug)
- fprintf(stderr, _("Table for %s, %s not found.
Keep as-is\n"),
- rc_dev.drv_name, rc_dev.keytable_name);
- return 0;
- }
- if (debug)
- fprintf(stderr, _("Table for %s, %s is on %s file.\n"),
- rc_dev.drv_name, rc_dev.keytable_name,
- cur->fname);
- if (cur->fname[0] == '/' || ((cur->fname[0] == '.') &&
strchr(cur->fname, '/'))) {
- fname = cur->fname;
- rc = parse_keyfile(fname, &name);
- if (rc < 0) {
- fprintf(stderr, _("Can't load %s table\n"),
fname);
- return -1;
- }
- } else {
- fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_USER_DIR) + 2);
- strcpy(fname, IR_KEYTABLE_USER_DIR);
- strcat(fname, "/");
- strcat(fname, cur->fname);
- rc = parse_keyfile(fname, &name);
- if (rc != 0) {
- fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_SYSTEM_DIR) + 2);
- strcpy(fname, IR_KEYTABLE_SYSTEM_DIR);
+ fprintf(stderr, _("Table for %s, %s is on %s
file.\n"),
+ rc_dev.drv_name, rc_dev.keytable_name,
+ cur->fname);
+ if (cur->fname[0] == '/' || ((cur->fname[0] == '.') &&
strchr(cur->fname, '/'))) {
+ fname = cur->fname;
+ rc = parse_keyfile(fname, &name);
+ if (rc < 0) {
+ fprintf(stderr, _("Can't load %s
table\n"), fname);
+ return -1;
+ }
+ } else {
+ fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_USER_DIR) + 2);
+ strcpy(fname, IR_KEYTABLE_USER_DIR);
strcat(fname, "/");
strcat(fname, cur->fname);
rc = parse_keyfile(fname, &name);
+ if (rc != 0) {
+ fname = malloc(strlen(cur->fname) +
strlen(IR_KEYTABLE_SYSTEM_DIR) + 2);
+ strcpy(fname, IR_KEYTABLE_SYSTEM_DIR);
+ strcat(fname, "/");
+ strcat(fname, cur->fname);
+ rc = parse_keyfile(fname, &name);
+ }
+ if (rc != 0) {
+ fprintf(stderr, _("Can't load %s table
from %s or %s\n"), cur->fname, IR_KEYTABLE_USER_DIR, IR_KEYTABLE_SYSTEM_DIR);
+ return -1;
+ }
}
- if (rc != 0) {
- fprintf(stderr, _("Can't load %s table from %s
or %s\n"), cur->fname, IR_KEYTABLE_USER_DIR, IR_KEYTABLE_SYSTEM_DIR);
+ if (!keytable) {
+ fprintf
Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag
On 11/16/2018 09:43 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>>
>> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
>> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>>
>> However, if userspace dup()ped the video device filehandle, then it is
>> possible to read or call DQBUF from two filehandles at the same time.
>>
>
> What side effects would reading have?
>
> As for another DQBUF in parallel, perhaps that's actually a valid
> operation that should be handled? I can imagine that one could want to
> have multiple threads dequeuing buffers as they become available, so
> that no dispatch thread is needed.
I think parallel DQBUFs can be done, but it has never been tested, nor
has vb2 been designed with that in mind. I also don't see the use-case
since if you have, say, two DQBUFs in parallel, then it will be random
which DQBUF gets which frame.
If we ever see a need for this, then that needs to be designed and tested
properly.
>
>> It is also possible to call REQBUFS from one filehandle while the other
>> is waiting for a buffer. This will remove all the buffers and reallocate
>> new ones. Removing all the buffers isn't the problem here (that's already
>> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
>> aware that the buffers have changed.
>>
>> This is fixed by setting a flag whenever the lock is released while waiting
>> for a buffer to arrive. And checking the flag where needed so we can return
>> -EBUSY.
>
> Maybe it would make more sense to actually handle those side effects?
> Such waiting DQBUF would then just fail in the same way as if it
> couldn't get a buffer (or if it's blocking, just retry until a correct
> buffer becomes available?).
That sounds like a good idea, but it isn't.
With this patch you can't call REQBUFS to reallocate buffers while a thread
is waiting for a buffer.
If I allow this, then the problem moves to when the thread that called REQBUFS
calls DQBUF next. Since we don't allow multiple DQBUFs this second DQBUF will
mysteriously fail. If we DO allow multiple DQBUFs, then how does REQBUFS ensure
that only the DQBUF that relied on the old buffers is stopped?
It sounds nice, but the more I think about it, the more problems I see with it.
I think it is perfectly reasonable to expect REQBUFS to return EBUSY if some
thread is still waiting for a buffer.
That said, I think one test is missing in vb2_core_create_bufs: there too it
should check waiting_in_dqbuf if q->num_buffers == 0: it is possible to do
REQBUFS(0) followed by CREATE_BUFS() while another thread is waiting for a
buffer. CREATE_BUFS acts like REQBUFS(count >= 1) in that case.
Admittedly, that would require some extremely unfortunate scheduling, but
it is easy enough to check this.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> .../media/common/videobuf2/videobuf2-core.c| 18 ++
>> include/media/videobuf2-core.h | 1 +
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 03954c13024c..138223af701f 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
>> vb2_memory memory,
>> return -EBUSY;
>> }
>>
>> + if (q->waiting_in_dqbuf && *count) {
>> + dprintk(1, "another dup()ped fd is waiting for a buffer\n");
>> + return -EBUSY;
>> + }
>> +
>> if (*count == 0 || q->num_buffers != 0 ||
>> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
>> /*
>> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue
>> *q, int nonblocking)
>> for (;;) {
>> int ret;
>>
>> + if (q->waiting_in_dqbuf) {
>> + dprintk(1, "another dup()ped fd is waiting for a
>> buffer\n");
>> + return -EBUSY;
>> + }
>> +
>> if (!q->streaming) {
>> dprintk(1, "streaming off, will not wait for
>> buffers\n");
>> return -EINVAL;
>> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
>> int nonblocking)
>> return -EAGAIN;
>> }
>>
>> + q->waiting_in_dqbuf = 1;
>> /*
>> * We are streaming and blocking, wait for another buffer to
>> * become ready or for streamoff. Driver's lock is released
>> to
>> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
>> int nonblocking)
>> * the locks or return an error if one occurred.
>> */
>>
Re: [PATCH 15/15] arm64: dts: allwinner: a64: Add Video Engine and reserved memory node
On Thu, Nov 15, 2018 at 03:50:13PM +0100, Paul Kocialkowski wrote:
> This adds nodes for the Video Engine and the associated reserved memory
> for the A64. Up to 96 MiB of memory are dedicated to the CMA pool.
>
> The pool is located at the end of the first 256 MiB of RAM so that the
> VPU can access it. It is unclear whether this is still a hard
> requirement for this platform, but it seems safer that way.
>
> Signed-off-by: Paul Kocialkowski
> ---
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 25 +++
> 1 file changed, 25 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 88b3e9110833..a35a5c9ffbbe 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -185,6 +185,20 @@
> (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> };
>
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + cma_pool: cma@4a00 {
> + compatible = "shared-dma-pool";
> + size = <0x600>;
> + alloc-ranges = <0x4a00 0x600>;
This introduces a DTC warning, since you're using a unit-address, but
don't have a reg register.
I've fixed it in the other DT by renaming that node to
default-pool. You can also drop the label, it's not used anywhere.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH 08/15] ARM/arm64: sunxi: Move H3/H5 syscon label over to soc-specific nodes
On Thu, Nov 15, 2018 at 03:50:06PM +0100, Paul Kocialkowski wrote:
> Now that we have specific nodes for the H3 and H5 system-controller
> that allow proper access to the EMAC clock configuration register,
> we no longer need a common dummy syscon node.
>
> Switch the syscon label over to each platform's dtsi file.
>
> Signed-off-by: Paul Kocialkowski
> ---
> arch/arm/boot/dts/sun8i-h3.dtsi | 2 +-
> arch/arm/boot/dts/sunxi-h3-h5.dtsi | 6 --
> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 2 +-
> 3 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 7157d954fb8c..b337a9282783 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -134,7 +134,7 @@
> };
>
> soc {
> - system-control@1c0 {
> + syscon: system-control@1c0 {
> compatible = "allwinner,sun8i-h3-system-control";
> reg = <0x01c0 0x1000>;
> #address-cells = <1>;
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index 4b1530ebe427..9175ff0fb59a 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -152,12 +152,6 @@
> };
> };
>
> - syscon: syscon@1c0 {
> - compatible = "allwinner,sun8i-h3-system-controller",
> - "syscon";
> - reg = <0x01c0 0x1000>;
> - };
> -
You're also dropping the syscon compatible there. But I'm not sure how
it could work with the H3 EMAC driver that would overwrite the
compatible already.
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH v2 8/9] phy: Add Cadence D-PHY support
Hi!
On Mon, Nov 12, 2018 at 03:21:56PM +0530, Kishon Vijay Abraham I wrote:
> > +static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode,
> > + union phy_configure_opts *opts)
> > +{
> > + struct cdns_dphy_cfg cfg = { 0 };
> > +
> > + if (mode != PHY_MODE_MIPI_DPHY)
> > + return -EINVAL;
> > +
> > + return cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > +}
> > +
> > +static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts
> > *opts)
> > +{
> > + struct cdns_dphy *dphy = phy_get_drvdata(phy);
> > + struct cdns_dphy_cfg cfg = { 0 };
> > + int ret;
> > +
> > + ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> > + if (ret)
> > + return ret;
>
> Can you explain why you need the same function to be invoked from both
> validate
> and configure callback? I see this to be redundant.
Sure.
Validate and configure serve two rather different purposes. validate
is here to make sure that a configuration can work with the PHY, and
to let the phy adjust the configuration to find a more optimal one.
configure, on the other hand, apply a configuration. We still have to
make sure that the configuration can work, since:
- We might have called validate any number of times, with any number
of configurations before calling configure, so we don't know which
configuration we validate is actually going to be applied later on
(if it's even applied)
- If we don't care about the validation at all, we might just call
configure directly
Does that make sense?
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCHv2 0/9] vb2/cedrus: add tag support
Hi Hans, On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil wrote: > > From: Hans Verkuil > > As was discussed here (among other places): > > https://lkml.org/lkml/2018/10/19/440 > > using capture queue buffer indices to refer to reference frames is > not a good idea. A better idea is to use a 'tag' where the > application can assign a u64 tag to an output buffer, which is then > copied to the capture buffer(s) derived from the output buffer. > Thanks for the patches. Please see my comments below. > A u64 is chosen since this allows userspace to also use pointers to > internal structures as 'tag'. > I think this is not true anymore in this version. > The first three patches add core tag support, the next patch document > the tag support, then a new helper function is added to v4l2-mem2mem.c > to easily copy data from a source to a destination buffer that drivers > can use. > > Next a new supports_tags vb2_queue flag is added to indicate that > the driver supports tags. Ideally this should not be necessary, but > that would require that all m2m drivers are converted to using the > new helper function introduced in the previous patch. That takes more > time then I have now since we want to get this in for 4.20. > > Finally the vim2m, vicodec and cedrus drivers are converted to support > tags. > > I also removed the 'pad' fields from the mpeg2 control structs (it > should never been added in the first place) and aligned the structs > to a u32 boundary (u64 for the tag values). u32 in this version > > Note that this might change further (Paul suggested using bitfields). > > Also note that the cedrus code doesn't set the sequence counter, that's > something that should still be added before this driver can be moved > out of staging. > > Note: if no buffer is found for a certain tag, then the dma address > is just set to 0. That happened before as well with invalid buffer > indices. This should be checked in the driver! Note that DMA address 0 may as well be a valid address. Should we have another way of signaling that? > > The previous RFC series was tested successfully with the cedrus driver. > > Regards, > > Hans > > Changes since v1: > > - changed to a u32 tag. Using a 64 bit tag was overly complicated due > to the bad layout of the v4l2_buffer struct, and there is no real > need for it by applications. I hope these won't become famous last words. :) For Chromium it should be okay indeed. Since we've been thinking about a redesign of the buffer struct, perhaps we can live with u32 for now and we can design the new struct to handle u64 nicely? Best regards, Tomasz
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On Fri, Nov 16, 2018 at 5:42 PM Hans Verkuil wrote:
>
> On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
> >>
> >> Calling the stop_streaming op can release the core serialization lock
> >> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
> >> An example of that behavior is the vivid driver.
> >
> > Why would vb2_queue->lock have to be released to wait for buffer to
> > finish? The drivers I worked with never had to do anything like that.
>
> Actually, they all do. It's done through the wait_prepare/finish callbacks
> and by setting those to vb2_ops_wait_prepare/finish.
>
> If you don't, then while one thread is waiting for a buffer to arrive,
> another thread cannot queue a new buffer since it will be serialized by
> queue->lock.
>
> v4l2-compliance even tests for this.
Why would you need the userspace to queue more buffers when you're
stopping the queue?
>
> >
> >>
> >> However, if userspace dup()ped the video device filehandle, then it is
> >> possible to stop streaming on one filehandle and call read/write or
> >> VIDIOC_QBUF from the other.
> >
> > How about other ioctls? I can imagine at least STREAMON could be
> > called at the same time too, but not sure if it would have any side
> > effects.
>
> STREAMON would return an error since q->streaming is still set while
> in the stop_streaming callback.
>
> So that combination is safe.
>
Okay, thanks. I'm still slightly worried that this approach with a
flag makes it possible to miss some non-trivial cases, though...
> Regards,
>
> Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> This is fixed by setting a flag whenever stop_streaming is called and
> >> checking the flag where needed so we can return -EBUSY.
> >>
> >> Signed-off-by: Hans Verkuil
> >> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
> >> ---
> >> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
> >> include/media/videobuf2-core.h | 1 +
> >> 2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> >> b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 138223af701f..560577321fe7 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> >> index, void *pb,
> >> dprintk(1, "fatal error occurred on queue\n");
> >> return -EIO;
> >> }
> >> + if (q->in_stop_streaming) {
> >> + dprintk(1, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >>
> >> vb = q->bufs[index];
> >>
> >> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >> * Tell driver to stop all transactions and release all queued
> >> * buffers.
> >> */
> >> - if (q->start_streaming_called)
> >> + if (q->start_streaming_called) {
> >> + q->in_stop_streaming = 1;
> >> call_void_qop(q, stop_streaming, q);
> >> + q->in_stop_streaming = 0;
> >> + }
> >>
> >> /*
> >> * If you see this warning, then the driver isn't cleaning up
> >> properly
> >> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> >> *q, char __user *data, size_
> >> return -EBUSY;
> >> }
> >>
> >> + if (q->in_stop_streaming) {
> >> + dprintk(3, "stop_streaming is called\n");
> >> + return -EBUSY;
> >> + }
> >> +
> >> /*
> >> * Initialize emulator on first call.
> >> */
> >> diff --git a/include/media/videobuf2-core.h
> >> b/include/media/videobuf2-core.h
> >> index 613f22910174..5a3d3ada5940 100644
> >> --- a/include/media/videobuf2-core.h
> >> +++ b/include/media/videobuf2-core.h
> >> @@ -585,6 +585,7 @@ struct vb2_queue {
> >> unsigned interror:1;
> >> unsigned intwaiting_for_buffers:1;
> >> unsigned intwaiting_in_dqbuf:1;
> >> + unsigned intin_stop_streaming:1;
> >> unsigned intis_multiplanar:1;
> >> unsigned intis_output:1;
> >> unsigned intcopy_timestamp:1;
> >> --
> >> 2.19.1
> >>
>
Re: [PATCH 1/2] vb2: add waiting_in_dqbuf flag
Hi Hans,
On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>
> Calling VIDIOC_DQBUF can release the core serialization lock pointed to
> by vb2_queue->lock if it has to wait for a new buffer to arrive.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to read or call DQBUF from two filehandles at the same time.
>
What side effects would reading have?
As for another DQBUF in parallel, perhaps that's actually a valid
operation that should be handled? I can imagine that one could want to
have multiple threads dequeuing buffers as they become available, so
that no dispatch thread is needed.
> It is also possible to call REQBUFS from one filehandle while the other
> is waiting for a buffer. This will remove all the buffers and reallocate
> new ones. Removing all the buffers isn't the problem here (that's already
> handled correctly by DQBUF), but the reallocating part is: DQBUF isn't
> aware that the buffers have changed.
>
> This is fixed by setting a flag whenever the lock is released while waiting
> for a buffer to arrive. And checking the flag where needed so we can return
> -EBUSY.
Maybe it would make more sense to actually handle those side effects?
Such waiting DQBUF would then just fail in the same way as if it
couldn't get a buffer (or if it's blocking, just retry until a correct
buffer becomes available?).
Best regards,
Tomasz
>
> Signed-off-by: Hans Verkuil
> ---
> .../media/common/videobuf2/videobuf2-core.c| 18 ++
> include/media/videobuf2-core.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 03954c13024c..138223af701f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -672,6 +672,11 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> vb2_memory memory,
> return -EBUSY;
> }
>
> + if (q->waiting_in_dqbuf && *count) {
> + dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> + return -EBUSY;
> + }
> +
> if (*count == 0 || q->num_buffers != 0 ||
> (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> /*
> @@ -1624,6 +1629,11 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> for (;;) {
> int ret;
>
> + if (q->waiting_in_dqbuf) {
> + dprintk(1, "another dup()ped fd is waiting for a
> buffer\n");
> + return -EBUSY;
> + }
> +
> if (!q->streaming) {
> dprintk(1, "streaming off, will not wait for
> buffers\n");
> return -EINVAL;
> @@ -1651,6 +1661,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> return -EAGAIN;
> }
>
> + q->waiting_in_dqbuf = 1;
> /*
> * We are streaming and blocking, wait for another buffer to
> * become ready or for streamoff. Driver's lock is released to
> @@ -1671,6 +1682,7 @@ static int __vb2_wait_for_done_vb(struct vb2_queue *q,
> int nonblocking)
> * the locks or return an error if one occurred.
> */
> call_void_qop(q, wait_finish, q);
> + q->waiting_in_dqbuf = 0;
> if (ret) {
> dprintk(1, "sleep was interrupted\n");
> return ret;
> @@ -2547,6 +2559,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> *q, char __user *data, size_
> if (!data)
> return -EINVAL;
>
> + if (q->waiting_in_dqbuf) {
> + dprintk(3, "another dup()ped fd is %s\n",
> + read ? "reading" : "writing");
> + return -EBUSY;
> + }
> +
> /*
> * Initialize emulator on first call.
> */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index e86981d615ae..613f22910174 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -584,6 +584,7 @@ struct vb2_queue {
> unsigned intstart_streaming_called:1;
> unsigned interror:1;
> unsigned intwaiting_for_buffers:1;
> + unsigned intwaiting_in_dqbuf:1;
> unsigned intis_multiplanar:1;
> unsigned intis_output:1;
> unsigned intcopy_timestamp:1;
> --
> 2.19.1
>
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
On 11/16/2018 09:34 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>>
>> Calling the stop_streaming op can release the core serialization lock
>> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
>> An example of that behavior is the vivid driver.
>
> Why would vb2_queue->lock have to be released to wait for buffer to
> finish? The drivers I worked with never had to do anything like that.
Actually, they all do. It's done through the wait_prepare/finish callbacks
and by setting those to vb2_ops_wait_prepare/finish.
If you don't, then while one thread is waiting for a buffer to arrive,
another thread cannot queue a new buffer since it will be serialized by
queue->lock.
v4l2-compliance even tests for this.
>
>>
>> However, if userspace dup()ped the video device filehandle, then it is
>> possible to stop streaming on one filehandle and call read/write or
>> VIDIOC_QBUF from the other.
>
> How about other ioctls? I can imagine at least STREAMON could be
> called at the same time too, but not sure if it would have any side
> effects.
STREAMON would return an error since q->streaming is still set while
in the stop_streaming callback.
So that combination is safe.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>
>> This is fixed by setting a flag whenever stop_streaming is called and
>> checking the flag where needed so we can return -EBUSY.
>>
>> Signed-off-by: Hans Verkuil
>> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
>> include/media/videobuf2-core.h | 1 +
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 138223af701f..560577321fe7 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
>> index, void *pb,
>> dprintk(1, "fatal error occurred on queue\n");
>> return -EIO;
>> }
>> + if (q->in_stop_streaming) {
>> + dprintk(1, "stop_streaming is called\n");
>> + return -EBUSY;
>> + }
>>
>> vb = q->bufs[index];
>>
>> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>> * Tell driver to stop all transactions and release all queued
>> * buffers.
>> */
>> - if (q->start_streaming_called)
>> + if (q->start_streaming_called) {
>> + q->in_stop_streaming = 1;
>> call_void_qop(q, stop_streaming, q);
>> + q->in_stop_streaming = 0;
>> + }
>>
>> /*
>> * If you see this warning, then the driver isn't cleaning up
>> properly
>> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
>> *q, char __user *data, size_
>> return -EBUSY;
>> }
>>
>> + if (q->in_stop_streaming) {
>> + dprintk(3, "stop_streaming is called\n");
>> + return -EBUSY;
>> + }
>> +
>> /*
>> * Initialize emulator on first call.
>> */
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 613f22910174..5a3d3ada5940 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -585,6 +585,7 @@ struct vb2_queue {
>> unsigned interror:1;
>> unsigned intwaiting_for_buffers:1;
>> unsigned intwaiting_in_dqbuf:1;
>> + unsigned intin_stop_streaming:1;
>> unsigned intis_multiplanar:1;
>> unsigned intis_output:1;
>> unsigned intcopy_timestamp:1;
>> --
>> 2.19.1
>>
Re: [PATCH 2/2] vb2: don't allow queueing buffers when canceling queue
Hi Hans,
On Wed, Nov 14, 2018 at 12:08 AM Hans Verkuil wrote:
>
> Calling the stop_streaming op can release the core serialization lock
> pointed to by vb2_queue->lock if it has to wait for buffers to finish.
> An example of that behavior is the vivid driver.
Why would vb2_queue->lock have to be released to wait for buffer to
finish? The drivers I worked with never had to do anything like that.
>
> However, if userspace dup()ped the video device filehandle, then it is
> possible to stop streaming on one filehandle and call read/write or
> VIDIOC_QBUF from the other.
How about other ioctls? I can imagine at least STREAMON could be
called at the same time too, but not sure if it would have any side
effects.
Best regards,
Tomasz
>
> This is fixed by setting a flag whenever stop_streaming is called and
> checking the flag where needed so we can return -EBUSY.
>
> Signed-off-by: Hans Verkuil
> Reported-by: syzbot+736c3aae4af7b50d9...@syzkaller.appspotmail.com
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 14 +-
> include/media/videobuf2-core.h | 1 +
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 138223af701f..560577321fe7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1503,6 +1503,10 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> index, void *pb,
> dprintk(1, "fatal error occurred on queue\n");
> return -EIO;
> }
> + if (q->in_stop_streaming) {
> + dprintk(1, "stop_streaming is called\n");
> + return -EBUSY;
> + }
>
> vb = q->bufs[index];
>
> @@ -1834,8 +1838,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> * Tell driver to stop all transactions and release all queued
> * buffers.
> */
> - if (q->start_streaming_called)
> + if (q->start_streaming_called) {
> + q->in_stop_streaming = 1;
> call_void_qop(q, stop_streaming, q);
> + q->in_stop_streaming = 0;
> + }
>
> /*
> * If you see this warning, then the driver isn't cleaning up properly
> @@ -2565,6 +2572,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue
> *q, char __user *data, size_
> return -EBUSY;
> }
>
> + if (q->in_stop_streaming) {
> + dprintk(3, "stop_streaming is called\n");
> + return -EBUSY;
> + }
> +
> /*
> * Initialize emulator on first call.
> */
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 613f22910174..5a3d3ada5940 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -585,6 +585,7 @@ struct vb2_queue {
> unsigned interror:1;
> unsigned intwaiting_for_buffers:1;
> unsigned intwaiting_in_dqbuf:1;
> + unsigned intin_stop_streaming:1;
> unsigned intis_multiplanar:1;
> unsigned intis_output:1;
> unsigned intcopy_timestamp:1;
> --
> 2.19.1
>
Re: media: ov8856: Add support for OV8856 sensor
Hi Ben, On Thu, Nov 08, 2018 at 11:41:46AM +0800, Ben Kao wrote: > This patch adds driver for Omnivision's ov8856 sensor, > the driver supports following features: > > - manual exposure/gain(analog and digital) control support > - two link frequencies > - VBLANK/HBLANK support > - test pattern support > - media controller support > - runtime pm support > - supported resolutions > + 3280x2464 at 30FPS > + 1640x1232 at 30FPS > > Signed-off-by: Ben Kao I just realised the driver is missing the MAINTAINERS entry. Could you provide one? Just the diff is fine, I'll then squash it to the patch. Thanks. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH 1/9] mm: Introduce new vm_insert_range API
On Fri, Nov 16, 2018 at 12:11 PM Matthew Wilcox wrote:
>
> On Fri, Nov 16, 2018 at 11:00:30AM +0530, Souptick Joarder wrote:
> > On Thu, Nov 15, 2018 at 11:44 PM Randy Dunlap wrote:
> > > On 11/15/18 7:45 AM, Souptick Joarder wrote:
> > > What is the opposite of vm_insert_range() or even of vm_insert_page()?
> > > or is there no need for that?
> >
> > There is no opposite function of vm_insert_range() / vm_insert_page().
> > My understanding is, in case of any error, mmap handlers will return the
> > err to user process and user space will decide the next action. So next
> > time when mmap handler is getting invoked it will map from the beginning.
> > Correct me if I am wrong.
>
> The opposite function, I suppose, is unmap_region().
>
> > > s/no./number/
> >
> > I didn't get it ??
>
> This is a 'sed' expression. 's' is the 'substitute' command; the /
> is a separator, 'no.' is what you wrote, and 'number' is what Randy
> is recommending instead.
Ok. Will change it in v2.
>
> > > > + for (i = 0; i < page_count; i++) {
> > > > + ret = vm_insert_page(vma, uaddr, pages[i]);
> > > > + if (ret < 0)
> > > > + return ret;
> > >
> > > For a non-trivial value of page_count:
> > > Is it a problem if vm_insert_page() succeeds for several pages
> > > and then fails?
> >
> > No, it will be considered as total failure and mmap handler will return
> > the err to user space.
>
> I think what Randy means is "What happens to the inserted pages?" and
> the answer is that mmap_region() jumps to the 'unmap_and_free_vma'
> label, which is an accurate name.
Sorry for incorrect understanding of the question.
