Message ID | 20191217032034.54897-13-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 1ih3Q0-000a3u-Ic; Tue, 17 Dec 2019 03:21:04 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727628AbfLQDV1 (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Mon, 16 Dec 2019 22:21:27 -0500 Received: from mail-pl1-f196.google.com ([209.85.214.196]:37568 "EHLO mail-pl1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727615AbfLQDV1 (ORCPT <rfc822;linux-media@vger.kernel.org>); Mon, 16 Dec 2019 22:21:27 -0500 Received: by mail-pl1-f196.google.com with SMTP id c23so5418919plz.4 for <linux-media@vger.kernel.org>; Mon, 16 Dec 2019 19:21:25 -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=w7BgC38Q++csLgKFC6p+4OJvyrlb3C/SOyZ7s22Frw0=; b=D6qNRxrNiByaYF3P2krgxc9qaMsAGuWOVyPKI+3IaZScIOd1bmthdyxh6cUySj7BoJ Gt7WM70dDxw55NQLonwaLHDv5sAiNSlwcxBtiFYufj7u47QEpb+59IChUhHUkRgUS5ku +IedKtqVE5v5xywRdfu1HByIpKnlv1C6JO1ao= 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=w7BgC38Q++csLgKFC6p+4OJvyrlb3C/SOyZ7s22Frw0=; b=ZVZ5RQEB2DrDktFBM1XLaMrB8/OeJpgZ0AY00Fx9mmrijxC3BswHkvLeExAbDcXWMJ Jqv1NLZ5kgaNZAlSV0cmXGvtN+PItbZ32vI420jYMEMJzWfLLvNBx6qSKThJn9M+phBV Ptevkt0xaJ3fVuGC50ejDsQh2MDtWBetqn5Tc5yVVgNgTCmP/o7O74GfFM1/KbeYgpn3 ogNXbussdUs3aPcYtpbNWBPVFDt5+fdfpaZQgYODl3rDRWKZVUu2zeN3svoHDVSSjd2g JLC3U1PxtCQjpmms6+CKO6iSk7UvU0b3FznEwNlT31H2Q7zSXNB4RmqJTsI9RZl9yXht 3bdQ== X-Gm-Message-State: APjAAAXx7TwkuQ+SMqaru6FOj0Kjcc2xEopMM4W6jPjNFUsJO2O/eU0R P8LXuwwUol4c+KOpDYsgFDLnig== X-Google-Smtp-Source: APXvYqzG2MAqV6YaHs0r4AKBZp8nz4G5wu16YR6deYslM0U/r044lJfXdUNlSJk7wL5nlXggt+GmlQ== X-Received: by 2002:a17:902:b418:: with SMTP id x24mr20126099plr.85.1576552885537; Mon, 16 Dec 2019 19:21:25 -0800 (PST) Received: from tigerii.tok.corp.google.com ([2401:fa00:8f:203:250d:e71d:5a0a:9afe]) by smtp.gmail.com with ESMTPSA id j3sm24387455pfi.8.2019.12.16.19.21.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Dec 2019 19:21:25 -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][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg Date: Tue, 17 Dec 2019 12:20:31 +0900 Message-Id: <20191217032034.54897-13-senozhatsky@chromium.org> X-Mailer: git-send-email 2.24.1.735.g03f4e72817-goog In-Reply-To: <20191217032034.54897-1-senozhatsky@chromium.org> References: <20191217032034.54897-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
Dec. 17, 2019, 3:20 a.m. UTC
Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
callbacks for cache synchronisation on exported buffers.
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
.../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
Comments
On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > Provide begin_cpu_access() and end_cpu_access() dma_buf_ops > callbacks for cache synchronisation on exported buffers. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > index 6db60e9d5183..bfc99a0cb7b9 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) > vb2_dma_sg_put(dbuf->priv); > } > There is no corresponding vb2_sg_buffer_consistent function here. Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no effect on dma-sg buffers. Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? I suspect it was just laziness in the past, and that it should be wired up, just as for dma-contig. Regards, Hans > +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, > + enum dma_data_direction direction) > +{ > + struct vb2_dma_sg_buf *buf = dbuf->priv; > + struct sg_table *sgt = buf->dma_sgt; > + > + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > + return 0; > +} > + > +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, > + enum dma_data_direction direction) > +{ > + struct vb2_dma_sg_buf *buf = dbuf->priv; > + struct sg_table *sgt = buf->dma_sgt; > + > + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > + return 0; > +} > + > static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) > { > struct vb2_dma_sg_buf *buf = dbuf->priv; > @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > .detach = vb2_dma_sg_dmabuf_ops_detach, > .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, > .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, > + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > .vmap = vb2_dma_sg_dmabuf_ops_vmap, > .mmap = vb2_dma_sg_dmabuf_ops_mmap, > .release = vb2_dma_sg_dmabuf_ops_release, >
On (20/01/10 11:13), Hans Verkuil wrote: > On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > > Provide begin_cpu_access() and end_cpu_access() dma_buf_ops > > callbacks for cache synchronisation on exported buffers. > > > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > --- > > .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index 6db60e9d5183..bfc99a0cb7b9 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) > > vb2_dma_sg_put(dbuf->priv); > > } > > > > There is no corresponding vb2_sg_buffer_consistent function here. > > Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs > argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no > effect on dma-sg buffers. > > Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? Laziness. > I suspect it was just laziness in the past, and that it should be wired > up, just as for dma-contig. OK, I can include it into v2. -ss
On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > > Provide begin_cpu_access() and end_cpu_access() dma_buf_ops > > callbacks for cache synchronisation on exported buffers. > > > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > > --- > > .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index 6db60e9d5183..bfc99a0cb7b9 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) > > vb2_dma_sg_put(dbuf->priv); > > } > > > > There is no corresponding vb2_sg_buffer_consistent function here. > > Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs > argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no > effect on dma-sg buffers. videobuf2-dma-sg allocates the memory using the page allocator directly, which means that there is no memory consistency guarantee. > > Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? > V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It isn't supposed to do anything for dma_map_sg_attrs(), which is only supposed to create the device (e.g. IOMMU) mapping for already allocated memory. > I suspect it was just laziness in the past, and that it should be wired > up, just as for dma-contig. > > Regards, > > Hans > > > +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, > > + enum dma_data_direction direction) > > +{ > > + struct vb2_dma_sg_buf *buf = dbuf->priv; > > + struct sg_table *sgt = buf->dma_sgt; > > + > > + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > > + return 0; > > +} > > + > > +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, > > + enum dma_data_direction direction) > > +{ > > + struct vb2_dma_sg_buf *buf = dbuf->priv; > > + struct sg_table *sgt = buf->dma_sgt; > > + > > + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > > + return 0; > > +} > > + > > static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) > > { > > struct vb2_dma_sg_buf *buf = dbuf->priv; > > @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > > .detach = vb2_dma_sg_dmabuf_ops_detach, > > .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, > > .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, > > + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > > + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > > .vmap = vb2_dma_sg_dmabuf_ops_vmap, > > .mmap = vb2_dma_sg_dmabuf_ops_mmap, > > .release = vb2_dma_sg_dmabuf_ops_release, > > >
On 1/28/20 5:38 AM, Tomasz Figa wrote: > On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: >>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops >>> callbacks for cache synchronisation on exported buffers. >>> >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> >>> --- >>> .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ >>> 1 file changed, 22 insertions(+) >>> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>> index 6db60e9d5183..bfc99a0cb7b9 100644 >>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) >>> vb2_dma_sg_put(dbuf->priv); >>> } >>> >> >> There is no corresponding vb2_sg_buffer_consistent function here. >> >> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs >> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no >> effect on dma-sg buffers. > > videobuf2-dma-sg allocates the memory using the page allocator > directly, which means that there is no memory consistency guarantee. > >> >> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? >> > > V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It > isn't supposed to do anything for dma_map_sg_attrs(), which is only > supposed to create the device (e.g. IOMMU) mapping for already > allocated memory. Ah, right. But could vb2_dma_sg_alloc_compacted() be modified so that is uses dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid question, I'm not an expert in this area. All I know is that I hate inconsistent APIs where something works for one thing, but not another. Regards, Hans > >> I suspect it was just laziness in the past, and that it should be wired >> up, just as for dma-contig. >> >> Regards, >> >> Hans >> >>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, >>> + enum dma_data_direction direction) >>> +{ >>> + struct vb2_dma_sg_buf *buf = dbuf->priv; >>> + struct sg_table *sgt = buf->dma_sgt; >>> + >>> + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); >>> + return 0; >>> +} >>> + >>> +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, >>> + enum dma_data_direction direction) >>> +{ >>> + struct vb2_dma_sg_buf *buf = dbuf->priv; >>> + struct sg_table *sgt = buf->dma_sgt; >>> + >>> + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); >>> + return 0; >>> +} >>> + >>> static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) >>> { >>> struct vb2_dma_sg_buf *buf = dbuf->priv; >>> @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { >>> .detach = vb2_dma_sg_dmabuf_ops_detach, >>> .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, >>> .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, >>> + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, >>> + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, >>> .vmap = vb2_dma_sg_dmabuf_ops_vmap, >>> .mmap = vb2_dma_sg_dmabuf_ops_mmap, >>> .release = vb2_dma_sg_dmabuf_ops_release, >>> >>
On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 1/28/20 5:38 AM, Tomasz Figa wrote: > > On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> > >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > >>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops > >>> callbacks for cache synchronisation on exported buffers. > >>> > >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > >>> --- > >>> .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ > >>> 1 file changed, 22 insertions(+) > >>> > >>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>> index 6db60e9d5183..bfc99a0cb7b9 100644 > >>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) > >>> vb2_dma_sg_put(dbuf->priv); > >>> } > >>> > >> > >> There is no corresponding vb2_sg_buffer_consistent function here. > >> > >> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs > >> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no > >> effect on dma-sg buffers. > > > > videobuf2-dma-sg allocates the memory using the page allocator > > directly, which means that there is no memory consistency guarantee. > > > >> > >> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? > >> > > > > V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It > > isn't supposed to do anything for dma_map_sg_attrs(), which is only > > supposed to create the device (e.g. IOMMU) mapping for already > > allocated memory. > > Ah, right. > > But could vb2_dma_sg_alloc_compacted() be modified so that is uses > dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid > question, I'm not an expert in this area. All I know is that I hate inconsistent > APIs where something works for one thing, but not another. > dma_alloc_attrs() would allocate contiguous memory, which kind of goes against the vb2_dma_sg allocator idea. Technically we could call it N times with size = 1 page to achieve what we want, but is this really what we want? Given that vb2_dma_sg has always been returning non-consistent memory, do we have any good reason to add consistent memory to it? If so, perhaps we could still do that in an incremental patch, to avoid complicating this already complex series? :) Best regards, Tomasz > Regards, > > Hans > > > > >> I suspect it was just laziness in the past, and that it should be wired > >> up, just as for dma-contig. > >> > >> Regards, > >> > >> Hans > >> > >>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, > >>> + enum dma_data_direction direction) > >>> +{ > >>> + struct vb2_dma_sg_buf *buf = dbuf->priv; > >>> + struct sg_table *sgt = buf->dma_sgt; > >>> + > >>> + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > >>> + return 0; > >>> +} > >>> + > >>> +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, > >>> + enum dma_data_direction direction) > >>> +{ > >>> + struct vb2_dma_sg_buf *buf = dbuf->priv; > >>> + struct sg_table *sgt = buf->dma_sgt; > >>> + > >>> + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > >>> + return 0; > >>> +} > >>> + > >>> static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) > >>> { > >>> struct vb2_dma_sg_buf *buf = dbuf->priv; > >>> @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > >>> .detach = vb2_dma_sg_dmabuf_ops_detach, > >>> .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, > >>> .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, > >>> + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > >>> + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > >>> .vmap = vb2_dma_sg_dmabuf_ops_vmap, > >>> .mmap = vb2_dma_sg_dmabuf_ops_mmap, > >>> .release = vb2_dma_sg_dmabuf_ops_release, > >>> > >> >
On 1/30/20 12:02 PM, Tomasz Figa wrote: > On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> On 1/28/20 5:38 AM, Tomasz Figa wrote: >>> On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >>>> >>>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: >>>>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops >>>>> callbacks for cache synchronisation on exported buffers. >>>>> >>>>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> >>>>> --- >>>>> .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>>>> index 6db60e9d5183..bfc99a0cb7b9 100644 >>>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c >>>>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) >>>>> vb2_dma_sg_put(dbuf->priv); >>>>> } >>>>> >>>> >>>> There is no corresponding vb2_sg_buffer_consistent function here. >>>> >>>> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs >>>> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no >>>> effect on dma-sg buffers. >>> >>> videobuf2-dma-sg allocates the memory using the page allocator >>> directly, which means that there is no memory consistency guarantee. >>> >>>> >>>> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? >>>> >>> >>> V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It >>> isn't supposed to do anything for dma_map_sg_attrs(), which is only >>> supposed to create the device (e.g. IOMMU) mapping for already >>> allocated memory. >> >> Ah, right. >> >> But could vb2_dma_sg_alloc_compacted() be modified so that is uses >> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid >> question, I'm not an expert in this area. All I know is that I hate inconsistent >> APIs where something works for one thing, but not another. >> > > dma_alloc_attrs() would allocate contiguous memory, which kind of goes > against the vb2_dma_sg allocator idea. Technically we could call it N > times with size = 1 page to achieve what we want, but is this really > what we want? > > Given that vb2_dma_sg has always been returning non-consistent memory, > do we have any good reason to add consistent memory to it? If so, > perhaps we could still do that in an incremental patch, to avoid > complicating this already complex series? :) I very much agree with that. But this should be very clearly documented. Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case? Regards, Hans > > Best regards, > Tomasz > >> Regards, >> >> Hans >> >>> >>>> I suspect it was just laziness in the past, and that it should be wired >>>> up, just as for dma-contig. >>>> >>>> Regards, >>>> >>>> Hans >>>> >>>>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, >>>>> + enum dma_data_direction direction) >>>>> +{ >>>>> + struct vb2_dma_sg_buf *buf = dbuf->priv; >>>>> + struct sg_table *sgt = buf->dma_sgt; >>>>> + >>>>> + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, >>>>> + enum dma_data_direction direction) >>>>> +{ >>>>> + struct vb2_dma_sg_buf *buf = dbuf->priv; >>>>> + struct sg_table *sgt = buf->dma_sgt; >>>>> + >>>>> + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); >>>>> + return 0; >>>>> +} >>>>> + >>>>> static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) >>>>> { >>>>> struct vb2_dma_sg_buf *buf = dbuf->priv; >>>>> @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { >>>>> .detach = vb2_dma_sg_dmabuf_ops_detach, >>>>> .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, >>>>> .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, >>>>> + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, >>>>> + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, >>>>> .vmap = vb2_dma_sg_dmabuf_ops_vmap, >>>>> .mmap = vb2_dma_sg_dmabuf_ops_mmap, >>>>> .release = vb2_dma_sg_dmabuf_ops_release, >>>>> >>>> >>
On Thu, Jan 30, 2020 at 9:18 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > On 1/30/20 12:02 PM, Tomasz Figa wrote: > > On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >> > >> On 1/28/20 5:38 AM, Tomasz Figa wrote: > >>> On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > >>>> > >>>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote: > >>>>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops > >>>>> callbacks for cache synchronisation on exported buffers. > >>>>> > >>>>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > >>>>> --- > >>>>> .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++ > >>>>> 1 file changed, 22 insertions(+) > >>>>> > >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>>>> index 6db60e9d5183..bfc99a0cb7b9 100644 > >>>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > >>>>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) > >>>>> vb2_dma_sg_put(dbuf->priv); > >>>>> } > >>>>> > >>>> > >>>> There is no corresponding vb2_sg_buffer_consistent function here. > >>>> > >>>> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs > >>>> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no > >>>> effect on dma-sg buffers. > >>> > >>> videobuf2-dma-sg allocates the memory using the page allocator > >>> directly, which means that there is no memory consistency guarantee. > >>> > >>>> > >>>> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()? > >>>> > >>> > >>> V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It > >>> isn't supposed to do anything for dma_map_sg_attrs(), which is only > >>> supposed to create the device (e.g. IOMMU) mapping for already > >>> allocated memory. > >> > >> Ah, right. > >> > >> But could vb2_dma_sg_alloc_compacted() be modified so that is uses > >> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid > >> question, I'm not an expert in this area. All I know is that I hate inconsistent > >> APIs where something works for one thing, but not another. > >> > > > > dma_alloc_attrs() would allocate contiguous memory, which kind of goes > > against the vb2_dma_sg allocator idea. Technically we could call it N > > times with size = 1 page to achieve what we want, but is this really > > what we want? > > > > Given that vb2_dma_sg has always been returning non-consistent memory, > > do we have any good reason to add consistent memory to it? If so, > > perhaps we could still do that in an incremental patch, to avoid > > complicating this already complex series? :) > > I very much agree with that. But this should be very clearly documented. > Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case? > Yes, IMHO that would make sense. My understanding is that currently the consistency of allocated memory is unspecified, so it can be either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can explicitly ask for inconsistent memory. Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent memory to avoid "optional" features or "hints" without guaranteed behavior. > Regards, > > Hans > > > > > Best regards, > > Tomasz > > > >> Regards, > >> > >> Hans > >> > >>> > >>>> I suspect it was just laziness in the past, and that it should be wired > >>>> up, just as for dma-contig. > >>>> > >>>> Regards, > >>>> > >>>> Hans > >>>> > >>>>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, > >>>>> + enum dma_data_direction direction) > >>>>> +{ > >>>>> + struct vb2_dma_sg_buf *buf = dbuf->priv; > >>>>> + struct sg_table *sgt = buf->dma_sgt; > >>>>> + > >>>>> + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, > >>>>> + enum dma_data_direction direction) > >>>>> +{ > >>>>> + struct vb2_dma_sg_buf *buf = dbuf->priv; > >>>>> + struct sg_table *sgt = buf->dma_sgt; > >>>>> + > >>>>> + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) > >>>>> { > >>>>> struct vb2_dma_sg_buf *buf = dbuf->priv; > >>>>> @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > >>>>> .detach = vb2_dma_sg_dmabuf_ops_detach, > >>>>> .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, > >>>>> .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, > >>>>> + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > >>>>> + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > >>>>> .vmap = vb2_dma_sg_dmabuf_ops_vmap, > >>>>> .mmap = vb2_dma_sg_dmabuf_ops_mmap, > >>>>> .release = vb2_dma_sg_dmabuf_ops_release, > >>>>> > >>>> > >> >
On (20/02/03 19:04), Tomasz Figa wrote: [..] > > I very much agree with that. But this should be very clearly documented. > > Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case? > > > > Yes, IMHO that would make sense. My understanding is that currently > the consistency of allocated memory is unspecified, so it can be > either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can > explicitly ask for inconsistent memory. > > Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when > V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent > memory to avoid "optional" features or "hints" without guaranteed > behavior. Documentation/DMA-attributes.txt says the following DMA_ATTR_NON_CONSISTENT ----------------------- DMA_ATTR_NON_CONSISTENT lets the platform to choose to return either consistent or non-consistent memory as it sees fit. By using this API, you are guaranteeing to the platform that you have all the correct and necessary sync points for this memory in the driver. -ss
On Tue, Feb 4, 2020 at 11:50 AM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > On (20/02/03 19:04), Tomasz Figa wrote: > [..] > > > I very much agree with that. But this should be very clearly documented. > > > Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case? > > > > > > > Yes, IMHO that would make sense. My understanding is that currently > > the consistency of allocated memory is unspecified, so it can be > > either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can > > explicitly ask for inconsistent memory. > > > > Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when > > V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent > > memory to avoid "optional" features or "hints" without guaranteed > > behavior. > > Documentation/DMA-attributes.txt says the following > > DMA_ATTR_NON_CONSISTENT > ----------------------- > > DMA_ATTR_NON_CONSISTENT lets the platform to choose to return either > consistent or non-consistent memory as it sees fit. By using this API, > you are guaranteeing to the platform that you have all the correct and > necessary sync points for this memory in the driver. Good point. And I also realized that some platforms just have no way to make the memory inconsistent, because they may have hardware coherency. Then we need to keep it a hint only. Best regards, Tomasz
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 6db60e9d5183..bfc99a0cb7b9 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf) vb2_dma_sg_put(dbuf->priv); } +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf, + enum dma_data_direction direction) +{ + struct vb2_dma_sg_buf *buf = dbuf->priv; + struct sg_table *sgt = buf->dma_sgt; + + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); + return 0; +} + +static int vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf, + enum dma_data_direction direction) +{ + struct vb2_dma_sg_buf *buf = dbuf->priv; + struct sg_table *sgt = buf->dma_sgt; + + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); + return 0; +} + static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf) { struct vb2_dma_sg_buf *buf = dbuf->priv; @@ -488,6 +508,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { .detach = vb2_dma_sg_dmabuf_ops_detach, .map_dma_buf = vb2_dma_sg_dmabuf_ops_map, .unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap, + .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, + .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, .vmap = vb2_dma_sg_dmabuf_ops_vmap, .mmap = vb2_dma_sg_dmabuf_ops_mmap, .release = vb2_dma_sg_dmabuf_ops_release,