Hi Sakari,
Few comments inline.
On Mon, May 8, 2017 at 11:03 PM, Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> From: Samu Onkalo <samu.onkalo@xxxxxxxxx>
>
> The user may request to the driver (vb2) to skip the cache maintenance
> operations in case the buffer does not need cache synchronisation, e.g. in
> cases where the buffer is passed between hardware blocks without it being
> touched by the CPU.
[snip]
> @@ -1199,6 +1236,11 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> dprintk(1, "buffer initialization failed\n");
> goto err;
> }
> +
> + /* This is new buffer memory --- always synchronise cache. */
> + __mem_prepare_planes(vb);
> + } else if (!no_cache_sync) {
> + __mem_prepare_planes(vb);
Do we actually need this at all for DMABUF, given that respective
callbacks in both vb2_dc and vb2_sg actually bail out if so?
> }
>
> ret = call_vb_qop(vb, buf_prepare, vb);
[snip]
> @@ -568,7 +571,11 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> }
>
> ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> - return ret ? ret : vb2_core_qbuf(q, b->index, b);
> + if (ret)
> + return ret;
> +
> + return vb2_core_qbuf(q, b->index, b,
> + b->flags & V4L2_BUF_FLAG_NO_CACHE_SYNC);
Can we really let the userspace alone control this? I believe there
are drivers that need to do some fixup in OUTPUT buffers before
sending to the hardware or in CAPTURE buffer after getting from the
hardware, respectively in buf_prepare or buf_finish. I believe such
driver needs to be able to override this behavior.
Actually I'm wondering if we really need this buffer flag at all.
Wouldn't the following work for typical use cases that we actually
care about performance of?
buffer_needs_cache_sync = (buffer_type_is_MMAP &&
buffer_is_non_coherent && (buffer_is_mmapped ||
buffer_has_kernel_mapping)) || buffer_is_USERPTR
The above should cover all the fast paths that are used only to
exchange data between devices, without the CPU involved, assuming that
drivers that don't need the fixups I mentioned before are properly
updated to request no kernel mapping for allocated buffers.
I've added (buffer_is_USERPTR) to the equation as it's really hard to
imagine a use case where there is no CPU access to the buffer, but
USERPTR needs to be used (instead of DMABUF). I might be missing
something, though.
Best regards,
Tomasz
