Message ID | 20200204025641.218376-3-senozhatsky@chromium.org (mailing list archive) |
---|---|
State | RFC, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1iyoNK-001Yl8-UB; Tue, 04 Feb 2020 02:55:43 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727190AbgBDC46 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 3 Feb 2020 21:56:58 -0500 Received: from mail-pl1-f194.google.com ([209.85.214.194]:42663 "EHLO mail-pl1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727160AbgBDC44 (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 3 Feb 2020 21:56:56 -0500 Received: by mail-pl1-f194.google.com with SMTP id e8so4034161plt.9 for <linux-media@vger.kernel.org>; Mon, 03 Feb 2020 18:56:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=0Dke3it4jAnnj8WZ58EOkoMCd5ygI2w9KJwtDCLTtPs=; b=WBTkzktPfQdyYJ/1Dp7tR7niJ57O1QuyCseggC0VAGq6KLGfx8m5m0cbGFUWSOl9oS 1b2VUGd1r6Vfy3ZaYWNwj7GS3vAKBbrDy6MCWr5GteDpDpOYRuYabQKVfXh9M2wJfAgV 1ENDVx0kml5b/I1+xEeFL9HJ8vL0yLWw/1UC8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=0Dke3it4jAnnj8WZ58EOkoMCd5ygI2w9KJwtDCLTtPs=; b=OzZRRQ//GyzOGyhWFIcIcLANUkAFepo7sQtzOlesRVvt+pPJsK5WsFZCsokMzmd0D3 P96fjYQ1v8Fzc/vsxMMcsETyv47quzOEe1/AJE3/hr93rlH+cff8MqaASnJImSxlbNXS lmMyjsEmr3269x2fQnV815sSozCpuZgsRIT+JDlMwUGWAaPYkBvKOgSclCA/eFYwFsO8 VKnV0cZLnj2KWhWvxQCi9GaoBPoOHpMpX54xyZSvyhuA5J4P3QEn8HBY+dXlkxin2dmj HFUEDImRcSXMD6E0VjijjjwLDPfpna/oXjAF4R8wxaJX8f7YJnprQC1M4QbOQFLyJ19u yAww== X-Gm-Message-State: APjAAAV2fvOQnwZCovq8XRq/kn2BVp79auk/oC7kxASi2XWLFSo5Ma0u mFIJdf9edt69HqtjMP5hGzF1gw== X-Google-Smtp-Source: APXvYqwFXZwVlWO8JJd9nP2tmu7pAXbLJCOTiydwVfbrp3yoOhZx3asfPWnN3Ku98Czzg/Yns7mIXg== X-Received: by 2002:a17:90a:fb41:: with SMTP id iq1mr3133875pjb.89.1580785016239; Mon, 03 Feb 2020 18:56:56 -0800 (PST) Received: from tigerii.tok.corp.google.com ([2401:fa00:8f:203:5bbb:c872:f2b1:f53b]) by smtp.gmail.com with ESMTPSA id e1sm22491971pfl.98.2020.02.03.18.56.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 03 Feb 2020 18:56:55 -0800 (PST) From: Sergey Senozhatsky <senozhatsky@chromium.org> To: Hans Verkuil <hans.verkuil@cisco.com>, Tomasz Figa <tfiga@chromium.org>, Mauro Carvalho Chehab <mchehab@kernel.org>, Kyungmin Park <kyungmin.park@samsung.com>, Marek Szyprowski <m.szyprowski@samsung.com> Cc: Sakari Ailus <sakari.ailus@iki.fi>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Pawel Osciak <posciak@chromium.org>, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky <senozhatsky@chromium.org> Subject: [RFC][PATCHv2 02/12] videobuf2: handle V4L2 buffer cache flags Date: Tue, 4 Feb 2020 11:56:31 +0900 Message-Id: <20200204025641.218376-3-senozhatsky@chromium.org> X-Mailer: git-send-email 2.25.0.341.g760bfbb309-goog In-Reply-To: <20200204025641.218376-1-senozhatsky@chromium.org> References: <20200204025641.218376-1-senozhatsky@chromium.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Series |
Implement V4L2_BUF_FLAG_NO_CACHE_* flags
|
|
Commit Message
Sergey Senozhatsky
Feb. 4, 2020, 2:56 a.m. UTC
Set video buffer cache management flags corresponding to V4L2 cache
flags.
Both ->prepare() and ->finish() cache management hints should be
passed during this stage (buffer preparation), because there is no
other way for user-space to skip ->finish() cache flush.
There are two possible alternative approaches:
- The first one is to move cache sync from ->finish() to dqbuf().
But this breaks some drivers, that need to fix-up buffers before
dequeueing them.
- The second one is to move ->finish() call from ->done() to dqbuf.
Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
.../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
Comments
On (20/02/04 11:56), Sergey Senozhatsky wrote: > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) > + return; > + > + vb->need_cache_sync_on_prepare = 1; > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} Last minute changes (tm), sorry. This is not right. ====8<====8<==== From: Sergey Senozhatsky <senozhatsky@chromium.org> Subject: [PATCH] videobuf2: handle V4L2 buffer cache flags Set video buffer cache management flags corresponding to V4L2 cache flags. Both ->prepare() and ->finish() cache management hints should be passed during this stage (buffer preparation), because there is no other way for user-space to skip ->finish() cache flush. There are two possible alternative approaches: - The first one is to move cache sync from ->finish() to dqbuf(). But this breaks some drivers, that need to fix-up buffers before dequeueing them. - The second one is to move ->finish() call from ->done() to dqbuf. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index eb5d5db96552..8ef57496b34a 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b return 0; } +static void set_buffer_cache_hints(struct vb2_queue *q, + struct vb2_buffer *vb, + struct v4l2_buffer *b) +{ + /* + * DMA exporter should take care of cache syncs, so we can avoid + * explicit ->prepare()/->finish() syncs. For other ->memory types + * we always need ->prepare() or/and ->finish() cache sync. + */ + if (q->memory == VB2_MEMORY_DMABUF) { + vb->need_cache_sync_on_finish = 0; + vb->need_cache_sync_on_prepare = 0; + return; + } + + vb->need_cache_sync_on_prepare = 1; + vb->need_cache_sync_on_finish = 1; + + if (!q->allow_cache_hints) + return; + + /* + * ->finish() cache sync can be avoided when queue direction is + * TO_DEVICE. + */ + if (q->dma_dir == DMA_TO_DEVICE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) + vb->need_cache_sync_on_prepare = 0; +} + static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, struct v4l2_buffer *b, bool is_prepare, struct media_request **p_req) @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md } if (!vb->prepared) { + set_buffer_cache_hints(q, vb, b); /* Copy relevant information provided by the userspace */ memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
On 2/4/20 3:56 AM, Sergey Senozhatsky wrote: > Set video buffer cache management flags corresponding to V4L2 cache > flags. > > Both ->prepare() and ->finish() cache management hints should be > passed during this stage (buffer preparation), because there is no > other way for user-space to skip ->finish() cache flush. > > There are two possible alternative approaches: > - The first one is to move cache sync from ->finish() to dqbuf(). > But this breaks some drivers, that need to fix-up buffers before > dequeueing them. > > - The second one is to move ->finish() call from ->done() to dqbuf. > > Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index eb5d5db96552..2da06a2ad6c4 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b > return 0; > } > > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) > + return; > + > + vb->need_cache_sync_on_prepare = 1; This needs a comment explaining why prepare is set to 1 by default. I remember we discussed this earlier, and the conclusion of that discussion needs to be documented here in a comment. Regards, Hans > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} > + > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, > struct v4l2_buffer *b, bool is_prepare, > struct media_request **p_req) > @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > } > > if (!vb->prepared) { > + set_buffer_cache_hints(q, vb, b); > /* Copy relevant information provided by the userspace */ > memset(vbuf->planes, 0, > sizeof(vbuf->planes[0]) * vb->num_planes); >
On (20/02/19 09:07), Hans Verkuil wrote: [..] > > +static void set_buffer_cache_hints(struct vb2_queue *q, > > + struct vb2_buffer *vb, > > + struct v4l2_buffer *b) > > +{ > > + /* > > + * DMA exporter should take care of cache syncs, so we can avoid > > + * explicit ->prepare()/->finish() syncs. For other ->memory types > > + * we always need ->prepare() or/and ->finish() cache sync. > > + */ > > + if (q->memory == VB2_MEMORY_DMABUF) { > > + vb->need_cache_sync_on_finish = 0; > > + vb->need_cache_sync_on_prepare = 0; > > + return; > > + } > > + > > + if (!q->allow_cache_hints) > > + return; > > + > > + vb->need_cache_sync_on_prepare = 1; > > This needs a comment explaining why prepare is set to 1 by default. I remember > we discussed this earlier, and the conclusion of that discussion needs to be > documented here in a comment. Please ignore this patch. There is a follow up which sets _both_ flags by default. The purpose is to preserve the existing behaviour, we can do all sorts of incremental changes (clear flags in more cases, etc.) later on. Do you want me to document this in the code? -ss
On 2/19/20 9:13 AM, Sergey Senozhatsky wrote: > On (20/02/19 09:07), Hans Verkuil wrote: > [..] >>> +static void set_buffer_cache_hints(struct vb2_queue *q, >>> + struct vb2_buffer *vb, >>> + struct v4l2_buffer *b) >>> +{ >>> + /* >>> + * DMA exporter should take care of cache syncs, so we can avoid >>> + * explicit ->prepare()/->finish() syncs. For other ->memory types >>> + * we always need ->prepare() or/and ->finish() cache sync. >>> + */ >>> + if (q->memory == VB2_MEMORY_DMABUF) { >>> + vb->need_cache_sync_on_finish = 0; >>> + vb->need_cache_sync_on_prepare = 0; >>> + return; >>> + } >>> + >>> + if (!q->allow_cache_hints) >>> + return; >>> + >>> + vb->need_cache_sync_on_prepare = 1; >> >> This needs a comment explaining why prepare is set to 1 by default. I remember >> we discussed this earlier, and the conclusion of that discussion needs to be >> documented here in a comment. > > Please ignore this patch. There is a follow up which sets _both_ > flags by default. The purpose is to preserve the existing behaviour, > we can do all sorts of incremental changes (clear flags in more cases, > etc.) later on. Do you want me to document this in the code? Yes please! Regards, Hans > > -ss >
On 2/4/20 3:56 AM, Sergey Senozhatsky wrote: > Set video buffer cache management flags corresponding to V4L2 cache > flags. > > Both ->prepare() and ->finish() cache management hints should be > passed during this stage (buffer preparation), because there is no > other way for user-space to skip ->finish() cache flush. > > There are two possible alternative approaches: > - The first one is to move cache sync from ->finish() to dqbuf(). > But this breaks some drivers, that need to fix-up buffers before > dequeueing them. > > - The second one is to move ->finish() call from ->done() to dqbuf. > > Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index eb5d5db96552..2da06a2ad6c4 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b > return 0; > } > > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) If q->allow_cache_hints is 0, then if userspace attempts to set these flags in v4l2_buffer, they should be cleared. That's to indicate to userspace that these flags won't work. That should be done in vb2_fill_vb2_v4l2_buffer(). Regards, Hans > + return; > + > + vb->need_cache_sync_on_prepare = 1; > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} > + > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, > struct v4l2_buffer *b, bool is_prepare, > struct media_request **p_req) > @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > } > > if (!vb->prepared) { > + set_buffer_cache_hints(q, vb, b); > /* Copy relevant information provided by the userspace */ > memset(vbuf->planes, 0, > sizeof(vbuf->planes[0]) * vb->num_planes); >
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index eb5d5db96552..2da06a2ad6c4 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b return 0; } +static void set_buffer_cache_hints(struct vb2_queue *q, + struct vb2_buffer *vb, + struct v4l2_buffer *b) +{ + /* + * DMA exporter should take care of cache syncs, so we can avoid + * explicit ->prepare()/->finish() syncs. For other ->memory types + * we always need ->prepare() or/and ->finish() cache sync. + */ + if (q->memory == VB2_MEMORY_DMABUF) { + vb->need_cache_sync_on_finish = 0; + vb->need_cache_sync_on_prepare = 0; + return; + } + + if (!q->allow_cache_hints) + return; + + vb->need_cache_sync_on_prepare = 1; + /* + * ->finish() cache sync can be avoided when queue direction is + * TO_DEVICE. + */ + if (q->dma_dir != DMA_TO_DEVICE) + vb->need_cache_sync_on_finish = 1; + else + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) + vb->need_cache_sync_on_prepare = 0; +} + static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, struct v4l2_buffer *b, bool is_prepare, struct media_request **p_req) @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md } if (!vb->prepared) { + set_buffer_cache_hints(q, vb, b); /* Copy relevant information provided by the userspace */ memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);