linux-kernel - Re: [PATCHv3] media: vb2: always set buffer cache sync hints

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] [day] [month] [year] [list]

Message-ID: <X8TNFwqthPkph/+p@jagdpanzerIV.localdomain>
Date:   Mon, 30 Nov 2020 19:44:39 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Tomasz Figa <tfiga@...omium.org>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCHv3] media: vb2: always set buffer cache sync hints

On (20/11/30 11:02), Hans Verkuil wrote:
> > @@ -414,6 +414,20 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> >  		vb->index = q->num_buffers + buffer;
> >  		vb->type = q->type;
> >  		vb->memory = memory;
> > +		/*
> > +		 * A workaround fix. We need to set these flags here so that
> > +		 * videobuf2 core will call ->prepare()/->finish() cache
> > +		 * sync/flush on vb2 buffers when appropriate. Otherwise, for
> > +		 * backends that don't rely on V4L2 (perhaps dvb) these flags
> > +		 * will always be false and, hence, videobuf2 core will skip
> > +		 * cache sync/flush operations. However, we can avoid explicit
> > +		 * ->prepare() and ->finish() cache sync for DMABUF buffers,
> > +		 * because DMA exporter takes care of it.
> > +		 */
> > +		if (q->memory != VB2_MEMORY_DMABUF) {
> > +			vb->need_cache_sync_on_prepare = 1;
> > +			vb->need_cache_sync_on_finish = 1;
> > +		}
> 
> Is this a work-around fix? Isn't this just a bug fix? It seems reasonable
> to always set this when allocating buffers for a queue. And for v4l2 these
> values can be changed if supported by the driver (allow_cache_hints is set).

Yes, it's a bug fix. It's a workaround bug fix in a sense that a proper
bug fix is to rename these flags and invert them (and patch all the
functions that set and test them). A sneak preview of what's in my tree:

---

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bd78f6c7f342..3b79042d9d5c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -264,10 +264,10 @@ struct vb2_buffer {
         *                      after the 'buf_finish' op is called.
         * copied_timestamp:    the timestamp of this capture buffer was copied
         *                      from an output buffer.
-        * need_cache_sync_on_prepare: when set buffer's ->prepare() function
-        *                      performs cache sync/invalidation.
-        * need_cache_sync_on_finish: when set buffer's ->finish() function
-        *                      performs cache sync/invalidation.
+        * no_cache_sync_on_prepare: when set buffer's ->prepare() function
+        *                      skips cache sync/invalidation.
+        * no_cache_sync_on_finish: when set buffer's ->finish() function
+        *                      skips cache sync/invalidation.
         * queued_entry:        entry on the queued buffers list, which holds
         *                      all buffers queued from userspace
         * done_entry:          entry on the list that stores all buffers ready
@@ -278,8 +278,8 @@ struct vb2_buffer {
        unsigned int            synced:1;
        unsigned int            prepared:1;
        unsigned int            copied_timestamp:1;
-       unsigned int            need_cache_sync_on_prepare:1;
-       unsigned int            need_cache_sync_on_finish:1;
+       unsigned int            no_cache_sync_on_prepare:1;
+       unsigned int            no_cache_sync_on_finish:1;
 
        struct vb2_plane        planes[VB2_MAX_PLANES];
        struct list_head        queued_entry;

---

and it's much bigger than a 3-liner ("workaround") fix.

> So the comment would be:
> 
> 	/*
> 	 * We need to set these flags here so that the videobuf2 core
> 	 * will call ->prepare()/->finish() cache sync/flush on vb2
> 	 * buffers when appropriate. However, we can avoid explicit
> 	 * ->prepare() and ->finish() cache sync for DMABUF buffers,
> 	 * because DMA exporter takes care of it.
> 	 */
> 
> The commit message would need to be modified as well.

OK.

	-ss