linux-kernel - Re: [PATCHv3 00/11] Implement V4L2_BUF_FLAG_NO_CACHE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <98205145-1a39-ce29-a2a0-c5abf8376349@xs4all.nl>
Date:   Thu, 27 Feb 2020 13:39:43 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Tomasz Figa <tfiga@...omium.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Pawel Osciak <posciak@...omium.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 00/11] Implement V4L2_BUF_FLAG_NO_CACHE_* flags

On 2/26/20 12:15 PM, Sergey Senozhatsky wrote:
> Hello,
> 
> 	V3 of the patchset, reshuffled and updated.
> 
> - Most notable changes:
> 
> a) Added a simple helper function vb2_queue_allows_cache_hints(),
>    which return true if queue has ->allow_cache_hints and when
>    ->memory is VB2_MEMORY_MMAP.
>    That is - user space cache and memory consistency hints are now
>    specifically for MMAP buffers and queues that support hints.
> 
> b) Set V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS capability bit only when queue
>    has ->allow_cache_hints and ->io_modes has VB2_MMAP bit set
> 
> c) Clear "incompatible" request's flags when queue does not permit
>    cache and memory consistency hints (IOW, when vb2_queue_allows_cache_hints()
>    return false)
> 
> 
> Minor changes:
> 
> - Squashed V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS and V4L2_FLAG_MEMORY_NON_CONSISTENT
>   patches.
> 
> - Added more documentation and code comments.
> 
> 
> Previous series:
> v2 link: https://lore.kernel.org/lkml/20200204025641.218376-1-senozhatsky@chromium.org/
> v1 link: https://lore.kernel.org/lkml/20191217032034.54897-1-senozhatsky@chromium.org/
> 
> 
> Series Intro
> ========================================================================
> 
>         This is a reworked version of the vb2 cache hints
> (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE / V4L2_BUF_FLAG_NO_CACHE_CLEAN)
> support patch series which previsouly was developed by Sakari and
> Laurent [0].
> 
> The patch set attempts to preserve the existing behvaiour - cache
> sync is performed in ->prepare() and ->finish() (unless the buffer
> is DMA exported). User space can request “default behavior” override
> with cache management hints, which are handled on a per-buffer basis
> and should be supplied with v4l2_buffer ->flags during buffer
> preparation. There are two possible hints:
> 
> - V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
>         No cache sync on ->finish()
> 
> - V4L2_BUF_FLAG_NO_CACHE_CLEAN
>         No cache sync on ->prepare()
> 
> In order to keep things on the safe side, we also require driver
> to explicitly state which of its queues (if any) support user space
> cache management hints (such queues should have ->allow_cache_hints
> bit set).
> 
> The patch set also (to some extent) simplifies allocators' ->prepare()
> and ->finish() callbacks. Namely, we move cache management decision
> making to the upper - core - layer. For example, if, previously, we
> would have something like this
> 
>         vb2_buffer_done()
>           vb2_dc_finish()
>             if (buf->db_attach)
>                return;
> 
> where each allocators' ->finish() callback would either bail
> out (DMA exported buffer, for instance) or sync, now that "bail
> out or sync" decision is made before we call into the allocator.
> 
> Along with cache management hints, user space is also able to
> adjust queue's memory consistency attributes. Memory consistency
> attribute (dma_attrs) is per-queue, yet it plays its role on the
> allocator level, when we allocate buffers’ private memory (planes).
> For the time being, only one consistency attribute is supported:
> DMA_ATTR_NON_CONSISTENT.

v3 looks very good, I only found some minor issues.

I think v4 should be ready to be merged, unless others have more comments.

Regards,

	Hans

> 
> [0] https://www.mail-archive.com/linux-media@vger.kernel.org/msg112459.html
> 
> Sergey Senozhatsky (11):
>   videobuf2: add cache management members
>   videobuf2: handle V4L2 buffer cache flags
>   videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
>   videobuf2: add queue memory consistency parameter
>   videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag
>   videobuf2: factor out planes prepare/finish functions
>   videobuf2: do not sync caches when we are allowed not to
>   videobuf2: check ->synced flag in prepare() and finish()
>   videobuf2: add begin/end cpu_access callbacks to dma-contig
>   videobuf2: add begin/end cpu_access callbacks to dma-sg
>   videobuf2: don't test db_attach in dma-contig prepare and finish
> 
>  Documentation/media/uapi/v4l/buffer.rst       |  29 +++++
>  .../media/uapi/v4l/vidioc-create-bufs.rst     |   8 +-
>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  22 +++-
>  .../media/common/videobuf2/videobuf2-core.c   | 110 +++++++++++++-----
>  .../common/videobuf2/videobuf2-dma-contig.c   |  39 ++++++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c |  36 ++++--
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  82 ++++++++++++-
>  drivers/media/dvb-core/dvb_vb2.c              |   2 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   5 +-
>  include/media/videobuf2-core.h                |  28 ++++-
>  include/uapi/linux/videodev2.h                |  11 +-
>  11 files changed, 314 insertions(+), 58 deletions(-)
>