On Fri, Nov 20, 2020 at 11:39 AM Hans Verkuil [off-list ref] wrote:

On 20/11/2020 10:18, Daniel Vetter wrote:

quoted

On Fri, Nov 20, 2020 at 9:28 AM Hans Verkuil [off-list ref] wrote:

quoted

On 20/11/2020 09:06, Hans Verkuil wrote:

quoted

On 19/11/2020 15:41, Daniel Vetter wrote:

quoted

The media model assumes that buffers are all preallocated, so that
when a media pipeline is running we never miss a deadline because the
buffers aren't allocated or available.

This means we cannot fix the v4l follow_pfn usage through
mmu_notifier, without breaking how this all works. The only real fix
is to deprecate userptr support for VM_IO | VM_PFNMAP mappings and
tell everyone to cut over to dma-buf memory sharing for zerocopy.

userptr for normal memory will keep working as-is, this only affects
the zerocopy userptr usage enabled in 50ac952d2263 ("[media]
videobuf2-dma-sg: Support io userptr operations on io memory").

Acked-by: Tomasz Figa <tfiga@chromium.org>

Acked-by: Hans Verkuil <redacted>

Actually, cancel this Acked-by.

So let me see if I understand this right: VM_IO | VM_PFNMAP mappings can
move around. There is a mmu_notifier that can be used to be notified when
that happens, but that can't be used with media buffers since those buffers
must always be available and in the same place.

So follow_pfn is replaced by unsafe_follow_pfn to signal that what is attempted
is unsafe and unreliable.

If CONFIG_STRICT_FOLLOW_PFN is set, then unsafe_follow_pfn will fail, if it
is unset, then it writes a warning to the kernel log but just continues while
still unsafe.

I am very much inclined to just drop VM_IO | VM_PFNMAP support in the media
subsystem. For vb2 there is a working alternative in the form of dmabuf, and
frankly for vb1 I don't care. If someone really needs this for a vb1 driver,
then they can do the work to convert that driver to vb2.

I've added Mauro to the CC list and I'll ping a few more people to see what
they think, but in my opinion support for USERPTR + VM_IO | VM_PFNMAP
should just be killed off.

If others would like to keep it, then frame_vector.c needs a comment before
the 'while' explaining why the unsafe_follow_pfn is there and that using
dmabuf is the proper alternative to use. That will make it easier for
developers to figure out why they see a kernel warning and what to do to
fix it, rather than having to dig through the git history for the reason.

I'm happy to add a comment, but otherwise if you all want to ditch
this, can we do this as a follow up on top? There's quite a bit of
code that can be deleted and I'd like to not hold up this patch set
here on that - it's already a fairly sprawling pain touching about 7
different subsystems (ok only 6-ish now since the s390 patch landed).
For the comment, is the explanation next to unsafe_follow_pfn not good
enough?

No, because that doesn't mention that you should use dma-buf as a replacement.
That's really the critical piece of information I'd like to see. That doesn't
belong in unsafe_follow_pfn, it needs to be in frame_vector.c since it's
vb2 specific.

Ah makes sense, I'll add that.

quoted

So ... can I get you to un-cancel your ack?

Hmm, I really would like to see support for this to be dropped completely.

How about this: just replace follow_pfn() by -EINVAL instead of unsafe_follow_pfn().

Add a TODO comment that this code now can be cleaned up a lot. Such a clean up patch
can be added on top later, and actually that is something that I can do once this
series has landed.

Regardless, frame_vector.c should mention dma-buf as a replacement in a comment
since I don't want users who hit this issue to have to dig through git logs
to find that dma-buf is the right approach.

BTW, nitpick: the subject line of this patch says 'videbuf' instead of 'videobuf'.

Will fix to, and next round will have the additional -EINVAL on top.
Iirc Mauro was pretty clear that we can't just delete this, so I kinda
don't want to get stuck in this discussion with my patches :-)
-Daniel

Regards,

        Hans

quoted

Thanks, Daniel

quoted

Regards,

        Hans

quoted

Thanks!

      Hans

quoted

Signed-off-by: Daniel Vetter <redacted>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Kees Cook <redacted>
Cc: Dan Williams <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Jérôme Glisse <redacted>
Cc: Jan Kara <jack@suse.cz>
Cc: Dan Williams <redacted>
Cc: linux-mm@kvack.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Cc: linux-media@vger.kernel.org
Cc: Pawel Osciak <redacted>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Tomasz Figa <tfiga@chromium.org>
Cc: Laurent Dufour <redacted>
Cc: Vlastimil Babka <redacted>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Michel Lespinasse <redacted>
Signed-off-by: Daniel Vetter <redacted>
--
v3:
- Reference the commit that enabled the zerocopy userptr use case to
  make it abundandtly clear that this patch only affects that, and not
  normal memory userptr. The old commit message already explained that
  normal memory userptr is unaffected, but I guess that was not clear
  enough.
---
 drivers/media/common/videobuf2/frame_vector.c | 2 +-
 drivers/media/v4l2-core/videobuf-dma-contig.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/common/videobuf2/frame_vector.c b/drivers/media/common/videobuf2/frame_vector.c
index a0e65481a201..1a82ec13ea00 100644
--- a/drivers/media/common/videobuf2/frame_vector.c
+++ b/drivers/media/common/videobuf2/frame_vector.c

@@ -70,7 +70,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames,
                     break;

             while (ret < nr_frames && start + PAGE_SIZE <= vma->vm_end) {
-                    err = follow_pfn(vma, start, &nums[ret]);
+                    err = unsafe_follow_pfn(vma, start, &nums[ret]);
                     if (err) {
                             if (ret == 0)
                                     ret = err;

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 52312ce2ba05..821c4a76ab96 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c

@@ -183,7 +183,7 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
     user_address = untagged_baddr;

     while (pages_done < (mem->size >> PAGE_SHIFT)) {
-            ret = follow_pfn(vma, user_address, &this_pfn);
+            ret = unsafe_follow_pfn(vma, user_address, &this_pfn);
             if (ret)
                     break;

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel