Hello,
Patch 1/7 has been sent already as an RFC but in this patchset
it can be found as a regular patch. The RFC version can be found
at [1] and also to prove its need look at [2]. Patch 2/7 fixes
Venus encoder issue with help of 1/7.
The other patches 3/7 to 7/7 can be treated as updates for
v4.14. For more info look in the patch descriptions.
Comments are welcome!
regards,
Stan
[1] https://lkml.org/lkml/2017/8/14/104
[2] https://patchwork.kernel.org/patch/9394145/
Stanimir Varbanov (7):
media: vb2: add bidirectional flag in vb2_queue
media: venus: venc: set correct resolution on compressed stream
media: venus: mark venc and vdec PM functions as __maybe_unused
media: venus: fill missing video_device name
media: venus: add helper to check supported codecs
media: venus: use helper function to check supported codecs
media: venus: venc: drop VP9 codec support
drivers/media/platform/qcom/venus/helpers.c | 49 +++++++++++++++++++++++++++++
drivers/media/platform/qcom/venus/helpers.h | 1 +
drivers/media/platform/qcom/venus/vdec.c | 31 +++++++++++-------
drivers/media/platform/qcom/venus/venc.c | 47 +++++++++++++++------------
drivers/media/v4l2-core/videobuf2-core.c | 17 +++++-----
include/media/videobuf2-core.h | 13 ++++++++
6 files changed, 118 insertions(+), 40 deletions(-)
--
2.11.0
This change is intended to give to the v4l2 drivers a choice to
change the default behavior of the v4l2-core DMA mapping direction
from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
Initially the issue with DMA mapping direction has been found in
Venus encoder driver where the hardware (firmware side) adds few
lines padding on bottom of the image buffer, and the consequence
is triggering of IOMMU protection faults.
This will help supporting venus encoder (and probably other drivers
in the future) which wants to map output type of buffers as
read/write.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
include/media/videobuf2-core.h | 13 +++++++++++++
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 0924594989b4..cb115ba6a1d2 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
void *mem_priv;
int plane;
int ret = -ENOMEM;
@@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
- q->dma_attrs, size, dma_dir, q->gfp_flags);
+ q->dma_attrs, size, q->dma_dir, q->gfp_flags);
if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
@@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
mem_priv = call_ptr_memop(vb, get_userptr,
q->alloc_devs[plane] ? : q->dev,
planes[plane].m.userptr,
- planes[plane].length, dma_dir);
+ planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed acquiring userspace memory for plane %d\n",
plane);
@@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(vb, attach_dmabuf,
q->alloc_devs[plane] ? : q->dev,
- dbuf, planes[plane].length, dma_dir);
+ dbuf, planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed to attach dmabuf\n");
ret = PTR_ERR(mem_priv);
@@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
+ if (q->bidirectional)
+ q->dma_dir = DMA_BIDIRECTIONAL;
+ else
+ q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+
return 0;
}
EXPORT_SYMBOL_GPL(vb2_core_queue_init);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cb97c224be73..ede09fff1de8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,17 @@ struct vb2_buf_ops {
* @dev: device to use for the default allocation context if the driver
* doesn't fill in the @alloc_devs array.
* @dma_attrs: DMA attributes to use for the DMA.
+ * @dma_dir: DMA mapping direction.
+ * @bidirectional: when this flag is set the DMA direction for the buffers of
+ * this queue will be overridden with DMA_BIDIRECTIONAL direction.
+ * This is useful in cases where the hardware (firmware) writes to
+ * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
+ * buffer which is mapped for write (DMA_FROM_DEVICE) in order
+ * to satisfy some internal hardware restrictions or adds a padding
+ * needed by the processing algorithm. In case the DMA mapping is
+ * not bidirectional but the hardware (firmware) trying to access
+ * the buffer (in the opposite direction) this could lead to an
+ * IOMMU protection faults.
* @fileio_read_once: report EOF after reading the first buffer
* @fileio_write_immediately: queue buffer after each write() call
* @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
@@ -495,6 +506,8 @@ struct vb2_queue {
unsigned int io_modes;
struct device *dev;
unsigned long dma_attrs;
+ enum dma_data_direction dma_dir;
+ unsigned bidirectional:1;
unsigned fileio_read_once:1;
unsigned fileio_write_immediately:1;
unsigned allow_zero_bytesused:1;
--
2.11.0
This change the alignment restriction for output type of buffers
only, also set corect input resolution and fill bidirectional
vb2 queue flag in order to map output type buffers read/write.
The last is needed by encoder firmware to add padding at the
bottom of output (input buffers).
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/venc.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 39748e7a08e4..01af1ac89edf 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -289,7 +289,7 @@ venc_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
pixmp->height = clamp(pixmp->height, inst->cap_height.min,
inst->cap_height.max);
- if (inst->core->res->hfi_version == HFI_VERSION_1XX)
+ if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
pixmp->height = ALIGN(pixmp->height, 32);
pixmp->width = ALIGN(pixmp->width, 2);
@@ -747,8 +747,8 @@ static int venc_init_session(struct venus_inst *inst)
if (ret)
return ret;
- ret = venus_helper_set_input_resolution(inst, inst->out_width,
- inst->out_height);
+ ret = venus_helper_set_input_resolution(inst, inst->width,
+ inst->height);
if (ret)
goto deinit;
@@ -1010,6 +1010,8 @@ static int m2m_queue_init(void *priv, struct vb2_queue *src_vq,
src_vq->allow_zero_bytesused = 1;
src_vq->min_buffers_needed = 1;
src_vq->dev = inst->core->dev;
+ if (inst->core->res->hfi_version == HFI_VERSION_1XX)
+ src_vq->bidirectional = 1;
ret = vb2_queue_init(src_vq);
if (ret)
return ret;
--
2.11.0
Without PM support gcc could warns about unused functions, thus
mark runtime_suspend/resume as __maybe_unused.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 6 ++----
drivers/media/platform/qcom/venus/venc.c | 6 ++----
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index eb0c1c51cfef..44d4848e878a 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1103,8 +1103,7 @@ static int vdec_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM
-static int vdec_runtime_suspend(struct device *dev)
+static __maybe_unused int vdec_runtime_suspend(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -1118,7 +1117,7 @@ static int vdec_runtime_suspend(struct device *dev)
return 0;
}
-static int vdec_runtime_resume(struct device *dev)
+static __maybe_unused int vdec_runtime_resume(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
int ret;
@@ -1132,7 +1131,6 @@ static int vdec_runtime_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops vdec_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 01af1ac89edf..4bffadd67238 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1226,8 +1226,7 @@ static int venc_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_PM
-static int venc_runtime_suspend(struct device *dev)
+static __maybe_unused int venc_runtime_suspend(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
@@ -1241,7 +1240,7 @@ static int venc_runtime_suspend(struct device *dev)
return 0;
}
-static int venc_runtime_resume(struct device *dev)
+static __maybe_unused int venc_runtime_resume(struct device *dev)
{
struct venus_core *core = dev_get_drvdata(dev);
int ret;
@@ -1255,7 +1254,6 @@ static int venc_runtime_resume(struct device *dev)
return ret;
}
-#endif
static const struct dev_pm_ops venc_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
--
2.11.0
Adds a helper function to runtime check supported encoder and
decoder codecs depending on venus version and platform.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/helpers.c | 49 +++++++++++++++++++++++++++++
drivers/media/platform/qcom/venus/helpers.h | 1 +
2 files changed, 50 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 5f4434c0a8f1..b52410deeb4c 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -34,6 +34,55 @@ struct intbuf {
unsigned long attrs;
};
+bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
+{
+ struct venus_core *core = inst->core;
+ u32 session_type = inst->session_type;
+ u32 codec;
+
+ switch (v4l2_pixfmt) {
+ case V4L2_PIX_FMT_H264:
+ codec = HFI_VIDEO_CODEC_H264;
+ break;
+ case V4L2_PIX_FMT_H263:
+ codec = HFI_VIDEO_CODEC_H263;
+ break;
+ case V4L2_PIX_FMT_MPEG1:
+ codec = HFI_VIDEO_CODEC_MPEG1;
+ break;
+ case V4L2_PIX_FMT_MPEG2:
+ codec = HFI_VIDEO_CODEC_MPEG2;
+ break;
+ case V4L2_PIX_FMT_MPEG4:
+ codec = HFI_VIDEO_CODEC_MPEG4;
+ break;
+ case V4L2_PIX_FMT_VC1_ANNEX_G:
+ case V4L2_PIX_FMT_VC1_ANNEX_L:
+ codec = HFI_VIDEO_CODEC_VC1;
+ break;
+ case V4L2_PIX_FMT_VP8:
+ codec = HFI_VIDEO_CODEC_VP8;
+ break;
+ case V4L2_PIX_FMT_VP9:
+ codec = HFI_VIDEO_CODEC_VP9;
+ break;
+ case V4L2_PIX_FMT_XVID:
+ codec = HFI_VIDEO_CODEC_DIVX;
+ break;
+ default:
+ return false;
+ }
+
+ if (session_type == VIDC_SESSION_TYPE_ENC && core->enc_codecs & codec)
+ return true;
+
+ if (session_type == VIDC_SESSION_TYPE_DEC && core->dec_codecs & codec)
+ return true;
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(venus_helper_check_codec);
+
static int intbufs_set_buffer(struct venus_inst *inst, u32 type)
{
struct venus_core *core = inst->core;
diff --git a/drivers/media/platform/qcom/venus/helpers.h b/drivers/media/platform/qcom/venus/helpers.h
index 6a061b417a93..971392be5df5 100644
--- a/drivers/media/platform/qcom/venus/helpers.h
+++ b/drivers/media/platform/qcom/venus/helpers.h
@@ -19,6 +19,7 @@
struct venus_inst;
+bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt);
struct vb2_v4l2_buffer *venus_helper_find_buf(struct venus_inst *inst,
unsigned int type, u32 idx);
void venus_helper_buffers_done(struct venus_inst *inst,
--
2.11.0
No one of the supported Venus version has implemented VP9 codec
for enconding, so drop it from the list of codecs.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/venc.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 9f459024aa07..6f123a387cf9 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -84,10 +84,6 @@ static const struct venus_format venc_formats[] = {
.pixfmt = V4L2_PIX_FMT_VP8,
.num_planes = 1,
.type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
- }, {
- .pixfmt = V4L2_PIX_FMT_VP9,
- .num_planes = 1,
- .type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE,
},
};
--
2.11.0
Use the helper function in decoder and encoder find_format
to runtime check supported codecs.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 24 +++++++++++++++++-------
drivers/media/platform/qcom/venus/venc.c | 28 +++++++++++++++++++---------
2 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 32a1feb2fe6a..da611a5eb670 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -102,7 +102,8 @@ static const struct venus_format vdec_formats[] = {
},
};
-static const struct venus_format *find_format(u32 pixfmt, u32 type)
+static const struct venus_format *
+find_format(struct venus_inst *inst, u32 pixfmt, u32 type)
{
const struct venus_format *fmt = vdec_formats;
unsigned int size = ARRAY_SIZE(vdec_formats);
@@ -116,11 +117,15 @@ static const struct venus_format *find_format(u32 pixfmt, u32 type)
if (i == size || fmt[i].type != type)
return NULL;
+ if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
+ !venus_helper_check_codec(inst, fmt[i].pixfmt))
+ return NULL;
+
return &fmt[i];
}
static const struct venus_format *
-find_format_by_index(unsigned int index, u32 type)
+find_format_by_index(struct venus_inst *inst, unsigned int index, u32 type)
{
const struct venus_format *fmt = vdec_formats;
unsigned int size = ARRAY_SIZE(vdec_formats);
@@ -140,6 +145,10 @@ find_format_by_index(unsigned int index, u32 type)
if (i == size)
return NULL;
+ if (type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE &&
+ !venus_helper_check_codec(inst, fmt[i].pixfmt))
+ return NULL;
+
return &fmt[i];
}
@@ -154,7 +163,7 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
- fmt = find_format(pixmp->pixelformat, f->type);
+ fmt = find_format(inst, pixmp->pixelformat, f->type);
if (!fmt) {
if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
pixmp->pixelformat = V4L2_PIX_FMT_NV12;
@@ -162,7 +171,7 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
pixmp->pixelformat = V4L2_PIX_FMT_H264;
else
return NULL;
- fmt = find_format(pixmp->pixelformat, f->type);
+ fmt = find_format(inst, pixmp->pixelformat, f->type);
pixmp->width = 1280;
pixmp->height = 720;
}
@@ -364,11 +373,12 @@ vdec_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
static int vdec_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
{
+ struct venus_inst *inst = to_inst(file);
const struct venus_format *fmt;
memset(f->reserved, 0, sizeof(f->reserved));
- fmt = find_format_by_index(f->index, f->type);
+ fmt = find_format_by_index(inst, f->index, f->type);
if (!fmt)
return -EINVAL;
@@ -417,10 +427,10 @@ static int vdec_enum_framesizes(struct file *file, void *fh,
struct venus_inst *inst = to_inst(file);
const struct venus_format *fmt;
- fmt = find_format(fsize->pixel_format,
+ fmt = find_format(inst, fsize->pixel_format,
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
if (!fmt) {
- fmt = find_format(fsize->pixel_format,
+ fmt = find_format(inst, fsize->pixel_format,
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
if (!fmt)
return -EINVAL;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index bcb50d39f88a..9f459024aa07 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -91,7 +91,8 @@ static const struct venus_format venc_formats[] = {
},
};
-static const struct venus_format *find_format(u32 pixfmt, u32 type)
+static const struct venus_format *
+find_format(struct venus_inst *inst, u32 pixfmt, u32 type)
{
const struct venus_format *fmt = venc_formats;
unsigned int size = ARRAY_SIZE(venc_formats);
@@ -105,11 +106,15 @@ static const struct venus_format *find_format(u32 pixfmt, u32 type)
if (i == size || fmt[i].type != type)
return NULL;
+ if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
+ !venus_helper_check_codec(inst, fmt[i].pixfmt))
+ return NULL;
+
return &fmt[i];
}
static const struct venus_format *
-find_format_by_index(unsigned int index, u32 type)
+find_format_by_index(struct venus_inst *inst, unsigned int index, u32 type)
{
const struct venus_format *fmt = venc_formats;
unsigned int size = ARRAY_SIZE(venc_formats);
@@ -129,6 +134,10 @@ find_format_by_index(unsigned int index, u32 type)
if (i == size)
return NULL;
+ if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
+ !venus_helper_check_codec(inst, fmt[i].pixfmt))
+ return NULL;
+
return &fmt[i];
}
@@ -246,9 +255,10 @@ venc_querycap(struct file *file, void *fh, struct v4l2_capability *cap)
static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
{
+ struct venus_inst *inst = to_inst(file);
const struct venus_format *fmt;
- fmt = find_format_by_index(f->index, f->type);
+ fmt = find_format_by_index(inst, f->index, f->type);
memset(f->reserved, 0, sizeof(f->reserved));
@@ -271,7 +281,7 @@ venc_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
- fmt = find_format(pixmp->pixelformat, f->type);
+ fmt = find_format(inst, pixmp->pixelformat, f->type);
if (!fmt) {
if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
pixmp->pixelformat = V4L2_PIX_FMT_H264;
@@ -279,7 +289,7 @@ venc_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
pixmp->pixelformat = V4L2_PIX_FMT_NV12;
else
return NULL;
- fmt = find_format(pixmp->pixelformat, f->type);
+ fmt = find_format(inst, pixmp->pixelformat, f->type);
pixmp->width = 1280;
pixmp->height = 720;
}
@@ -524,10 +534,10 @@ static int venc_enum_framesizes(struct file *file, void *fh,
fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
- fmt = find_format(fsize->pixel_format,
+ fmt = find_format(inst, fsize->pixel_format,
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
if (!fmt) {
- fmt = find_format(fsize->pixel_format,
+ fmt = find_format(inst, fsize->pixel_format,
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
if (!fmt)
return -EINVAL;
@@ -554,10 +564,10 @@ static int venc_enum_frameintervals(struct file *file, void *fh,
fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
- fmt = find_format(fival->pixel_format,
+ fmt = find_format(inst, fival->pixel_format,
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
if (!fmt) {
- fmt = find_format(fival->pixel_format,
+ fmt = find_format(inst, fival->pixel_format,
V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
if (!fmt)
return -EINVAL;
--
2.11.0
This fills missing (forgotten) video device name with
appropriate string so that udev can distinguishes between
decoder and encoder devices.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
drivers/media/platform/qcom/venus/vdec.c | 1 +
drivers/media/platform/qcom/venus/venc.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 44d4848e878a..32a1feb2fe6a 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -1069,6 +1069,7 @@ static int vdec_probe(struct platform_device *pdev)
if (!vdev)
return -ENOMEM;
+ strlcpy(vdev->name, "qcom-venus-decoder", sizeof(vdev->name));
vdev->release = video_device_release;
vdev->fops = &vdec_fops;
vdev->ioctl_ops = &vdec_ioctl_ops;
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 4bffadd67238..bcb50d39f88a 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -1192,6 +1192,7 @@ static int venc_probe(struct platform_device *pdev)
if (!vdev)
return -ENOMEM;
+ strlcpy(vdev->name, "qcom-venus-encoder", sizeof(vdev->name));
vdev->release = video_device_release;
vdev->fops = &venc_fops;
vdev->ioctl_ops = &venc_ioctl_ops;
--
2.11.0
On 08/18/17 16:16, Stanimir Varbanov wrote:
> This change is intended to give to the v4l2 drivers a choice to
> change the default behavior of the v4l2-core DMA mapping direction
> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>
> Initially the issue with DMA mapping direction has been found in
> Venus encoder driver where the hardware (firmware side) adds few
> lines padding on bottom of the image buffer, and the consequence
> is triggering of IOMMU protection faults.
>
> This will help supporting venus encoder (and probably other drivers
> in the future) which wants to map output type of buffers as
> read/write.
>
> Signed-off-by: Stanimir Varbanov <[email protected]>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
> include/media/videobuf2-core.h | 13 +++++++++++++
> 2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 0924594989b4..cb115ba6a1d2 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> void *mem_priv;
> int plane;
> int ret = -ENOMEM;
> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>
> mem_priv = call_ptr_memop(vb, alloc,
> q->alloc_devs[plane] ? : q->dev,
> - q->dma_attrs, size, dma_dir, q->gfp_flags);
> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> if (IS_ERR_OR_NULL(mem_priv)) {
> if (mem_priv)
> ret = PTR_ERR(mem_priv);
> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> mem_priv = call_ptr_memop(vb, get_userptr,
> q->alloc_devs[plane] ? : q->dev,
> planes[plane].m.userptr,
> - planes[plane].length, dma_dir);
> + planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed acquiring userspace memory for plane %d\n",
> plane);
> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(vb, attach_dmabuf,
> q->alloc_devs[plane] ? : q->dev,
> - dbuf, planes[plane].length, dma_dir);
> + dbuf, planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed to attach dmabuf\n");
> ret = PTR_ERR(mem_priv);
> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> + if (q->bidirectional)
> + q->dma_dir = DMA_BIDIRECTIONAL;
> + else
> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..ede09fff1de8 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,17 @@ struct vb2_buf_ops {
> * @dev: device to use for the default allocation context if the driver
> * doesn't fill in the @alloc_devs array.
> * @dma_attrs: DMA attributes to use for the DMA.
> + * @dma_dir: DMA mapping direction.
This one should be moved to the 'Private elements' section. This is set and used
by the vb2 core, drivers won't set this.
Looks good otherwise.
Regards,
Hans
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + * this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + * This is useful in cases where the hardware (firmware) writes to
> + * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + * buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + * to satisfy some internal hardware restrictions or adds a padding
> + * needed by the processing algorithm. In case the DMA mapping is
> + * not bidirectional but the hardware (firmware) trying to access
> + * the buffer (in the opposite direction) this could lead to an
> + * IOMMU protection faults.
> * @fileio_read_once: report EOF after reading the first buffer
> * @fileio_write_immediately: queue buffer after each write() call
> * @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
> @@ -495,6 +506,8 @@ struct vb2_queue {
> unsigned int io_modes;
> struct device *dev;
> unsigned long dma_attrs;
> + enum dma_data_direction dma_dir;
> + unsigned bidirectional:1;
> unsigned fileio_read_once:1;
> unsigned fileio_write_immediately:1;
> unsigned allow_zero_bytesused:1;
>
Hi Hans,
On 08/18/2017 05:30 PM, Hans Verkuil wrote:
> On 08/18/17 16:16, Stanimir Varbanov wrote:
>> This change is intended to give to the v4l2 drivers a choice to
>> change the default behavior of the v4l2-core DMA mapping direction
>> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
>> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>>
>> Initially the issue with DMA mapping direction has been found in
>> Venus encoder driver where the hardware (firmware side) adds few
>> lines padding on bottom of the image buffer, and the consequence
>> is triggering of IOMMU protection faults.
>>
>> This will help supporting venus encoder (and probably other drivers
>> in the future) which wants to map output type of buffers as
>> read/write.
>>
>> Signed-off-by: Stanimir Varbanov <[email protected]>
>> ---
>> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
>> include/media/videobuf2-core.h | 13 +++++++++++++
>> 2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 0924594989b4..cb115ba6a1d2 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
>> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>> {
>> struct vb2_queue *q = vb->vb2_queue;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> void *mem_priv;
>> int plane;
>> int ret = -ENOMEM;
>> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>
>> mem_priv = call_ptr_memop(vb, alloc,
>> q->alloc_devs[plane] ? : q->dev,
>> - q->dma_attrs, size, dma_dir, q->gfp_flags);
>> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
>> if (IS_ERR_OR_NULL(mem_priv)) {
>> if (mem_priv)
>> ret = PTR_ERR(mem_priv);
>> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
>> void *mem_priv;
>> unsigned int plane;
>> int ret = 0;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> bool reacquired = vb->planes[0].mem_priv == NULL;
>>
>> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
>> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
>> mem_priv = call_ptr_memop(vb, get_userptr,
>> q->alloc_devs[plane] ? : q->dev,
>> planes[plane].m.userptr,
>> - planes[plane].length, dma_dir);
>> + planes[plane].length, q->dma_dir);
>> if (IS_ERR(mem_priv)) {
>> dprintk(1, "failed acquiring userspace memory for plane %d\n",
>> plane);
>> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>> void *mem_priv;
>> unsigned int plane;
>> int ret = 0;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> bool reacquired = vb->planes[0].mem_priv == NULL;
>>
>> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
>> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>> /* Acquire each plane's memory */
>> mem_priv = call_ptr_memop(vb, attach_dmabuf,
>> q->alloc_devs[plane] ? : q->dev,
>> - dbuf, planes[plane].length, dma_dir);
>> + dbuf, planes[plane].length, q->dma_dir);
>> if (IS_ERR(mem_priv)) {
>> dprintk(1, "failed to attach dmabuf\n");
>> ret = PTR_ERR(mem_priv);
>> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
>> if (q->buf_struct_size == 0)
>> q->buf_struct_size = sizeof(struct vb2_buffer);
>>
>> + if (q->bidirectional)
>> + q->dma_dir = DMA_BIDIRECTIONAL;
>> + else
>> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index cb97c224be73..ede09fff1de8 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -427,6 +427,17 @@ struct vb2_buf_ops {
>> * @dev: device to use for the default allocation context if the driver
>> * doesn't fill in the @alloc_devs array.
>> * @dma_attrs: DMA attributes to use for the DMA.
>> + * @dma_dir: DMA mapping direction.
>
> This one should be moved to the 'Private elements' section. This is set and used
> by the vb2 core, drivers won't set this.
>
> Looks good otherwise.
>
Ah, correct, will fix that. Thanks!
--
regards,
Stan
This change is intended to give to the v4l2 drivers a choice to
change the default behavior of the v4l2-core DMA mapping direction
from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
Initially the issue with DMA mapping direction has been found in
Venus encoder driver where the hardware (firmware side) adds few
lines padding on bottom of the image buffer, and the consequence
is triggering of IOMMU protection faults.
This will help supporting venus encoder (and probably other drivers
in the future) which wants to map output type of buffers as
read/write.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
v2: move dma_dir into private section.
drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
include/media/videobuf2-core.h | 13 +++++++++++++
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 0924594989b4..cb115ba6a1d2 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
void *mem_priv;
int plane;
int ret = -ENOMEM;
@@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
- q->dma_attrs, size, dma_dir, q->gfp_flags);
+ q->dma_attrs, size, q->dma_dir, q->gfp_flags);
if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
@@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
mem_priv = call_ptr_memop(vb, get_userptr,
q->alloc_devs[plane] ? : q->dev,
planes[plane].m.userptr,
- planes[plane].length, dma_dir);
+ planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed acquiring userspace memory for plane %d\n",
plane);
@@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(vb, attach_dmabuf,
q->alloc_devs[plane] ? : q->dev,
- dbuf, planes[plane].length, dma_dir);
+ dbuf, planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed to attach dmabuf\n");
ret = PTR_ERR(mem_priv);
@@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
+ if (q->bidirectional)
+ q->dma_dir = DMA_BIDIRECTIONAL;
+ else
+ q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+
return 0;
}
EXPORT_SYMBOL_GPL(vb2_core_queue_init);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cb97c224be73..ef9b64398c8c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,16 @@ struct vb2_buf_ops {
* @dev: device to use for the default allocation context if the driver
* doesn't fill in the @alloc_devs array.
* @dma_attrs: DMA attributes to use for the DMA.
+ * @bidirectional: when this flag is set the DMA direction for the buffers of
+ * this queue will be overridden with DMA_BIDIRECTIONAL direction.
+ * This is useful in cases where the hardware (firmware) writes to
+ * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
+ * buffer which is mapped for write (DMA_FROM_DEVICE) in order
+ * to satisfy some internal hardware restrictions or adds a padding
+ * needed by the processing algorithm. In case the DMA mapping is
+ * not bidirectional but the hardware (firmware) trying to access
+ * the buffer (in the opposite direction) this could lead to an
+ * IOMMU protection faults.
* @fileio_read_once: report EOF after reading the first buffer
* @fileio_write_immediately: queue buffer after each write() call
* @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
@@ -465,6 +475,7 @@ struct vb2_buf_ops {
* Private elements (won't appear at the uAPI book):
* @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
* @memory: current memory type used
+ * @dma_dir: DMA mapping direction.
* @bufs: videobuf buffer structures
* @num_buffers: number of allocated/used buffers
* @queued_list: list of buffers currently queued from userspace
@@ -495,6 +506,7 @@ struct vb2_queue {
unsigned int io_modes;
struct device *dev;
unsigned long dma_attrs;
+ unsigned bidirectional:1;
unsigned fileio_read_once:1;
unsigned fileio_write_immediately:1;
unsigned allow_zero_bytesused:1;
@@ -516,6 +528,7 @@ struct vb2_queue {
/* private: internal use only */
struct mutex mmap_lock;
unsigned int memory;
+ enum dma_data_direction dma_dir;
struct vb2_buffer *bufs[VB2_MAX_FRAME];
unsigned int num_buffers;
--
2.11.0
Hi Stanimir,
On 2017-08-21 11:09, Stanimir Varbanov wrote:
> This change is intended to give to the v4l2 drivers a choice to
> change the default behavior of the v4l2-core DMA mapping direction
> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>
> Initially the issue with DMA mapping direction has been found in
> Venus encoder driver where the hardware (firmware side) adds few
> lines padding on bottom of the image buffer, and the consequence
> is triggering of IOMMU protection faults.
>
> This will help supporting venus encoder (and probably other drivers
> in the future) which wants to map output type of buffers as
> read/write.
>
> Signed-off-by: Stanimir Varbanov <[email protected]>
This has been already discussed about a year ago, but it got lost in
meantime, probably due to lack of users. I hope that this time it
finally will get into vb2.
For the reference, see https://patchwork.kernel.org/patch/9388163/
> ---
> v2: move dma_dir into private section.
>
> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
> include/media/videobuf2-core.h | 13 +++++++++++++
> 2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 0924594989b4..cb115ba6a1d2 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> void *mem_priv;
> int plane;
> int ret = -ENOMEM;
> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>
> mem_priv = call_ptr_memop(vb, alloc,
> q->alloc_devs[plane] ? : q->dev,
> - q->dma_attrs, size, dma_dir, q->gfp_flags);
> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> if (IS_ERR_OR_NULL(mem_priv)) {
> if (mem_priv)
> ret = PTR_ERR(mem_priv);
> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> mem_priv = call_ptr_memop(vb, get_userptr,
> q->alloc_devs[plane] ? : q->dev,
> planes[plane].m.userptr,
> - planes[plane].length, dma_dir);
> + planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed acquiring userspace memory for plane %d\n",
> plane);
> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(vb, attach_dmabuf,
> q->alloc_devs[plane] ? : q->dev,
> - dbuf, planes[plane].length, dma_dir);
> + dbuf, planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed to attach dmabuf\n");
> ret = PTR_ERR(mem_priv);
> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> + if (q->bidirectional)
> + q->dma_dir = DMA_BIDIRECTIONAL;
> + else
> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..ef9b64398c8c 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
> * @dev: device to use for the default allocation context if the driver
> * doesn't fill in the @alloc_devs array.
> * @dma_attrs: DMA attributes to use for the DMA.
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + * this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + * This is useful in cases where the hardware (firmware) writes to
> + * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + * buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + * to satisfy some internal hardware restrictions or adds a padding
> + * needed by the processing algorithm. In case the DMA mapping is
> + * not bidirectional but the hardware (firmware) trying to access
> + * the buffer (in the opposite direction) this could lead to an
> + * IOMMU protection faults.
> * @fileio_read_once: report EOF after reading the first buffer
> * @fileio_write_immediately: queue buffer after each write() call
> * @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
> @@ -465,6 +475,7 @@ struct vb2_buf_ops {
> * Private elements (won't appear at the uAPI book):
> * @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
> * @memory: current memory type used
> + * @dma_dir: DMA mapping direction.
> * @bufs: videobuf buffer structures
> * @num_buffers: number of allocated/used buffers
> * @queued_list: list of buffers currently queued from userspace
> @@ -495,6 +506,7 @@ struct vb2_queue {
> unsigned int io_modes;
> struct device *dev;
> unsigned long dma_attrs;
> + unsigned bidirectional:1;
> unsigned fileio_read_once:1;
> unsigned fileio_write_immediately:1;
> unsigned allow_zero_bytesused:1;
> @@ -516,6 +528,7 @@ struct vb2_queue {
> /* private: internal use only */
> struct mutex mmap_lock;
> unsigned int memory;
> + enum dma_data_direction dma_dir;
> struct vb2_buffer *bufs[VB2_MAX_FRAME];
> unsigned int num_buffers;
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On 08/21/2017 11:29 AM, Marek Szyprowski wrote:
> Hi Stanimir,
>
> On 2017-08-21 11:09, Stanimir Varbanov wrote:
>> This change is intended to give to the v4l2 drivers a choice to
>> change the default behavior of the v4l2-core DMA mapping direction
>> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
>> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>>
>> Initially the issue with DMA mapping direction has been found in
>> Venus encoder driver where the hardware (firmware side) adds few
>> lines padding on bottom of the image buffer, and the consequence
>> is triggering of IOMMU protection faults.
>>
>> This will help supporting venus encoder (and probably other drivers
>> in the future) which wants to map output type of buffers as
>> read/write.
>>
>> Signed-off-by: Stanimir Varbanov <[email protected]>
>
> This has been already discussed about a year ago, but it got lost in
> meantime, probably due to lack of users. I hope that this time it
> finally will get into vb2.
>
> For the reference, see https://patchwork.kernel.org/patch/9388163/
Interesting.
Stanimir, I like your implementation better than the macro in the old
patch. But as it said there, videobuf2-dma-sg/contig/vmalloc.c have references
to DMA_FROM_DEVICE that won't work with BIDIRECTIONAL, so those need
to be adapted as well. I missed that when I reviewed your patch :-(
Regards,
Hans
>
>> ---
>> v2: move dma_dir into private section.
>>
>> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
>> include/media/videobuf2-core.h | 13 +++++++++++++
>> 2 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 0924594989b4..cb115ba6a1d2 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
>> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>> {
>> struct vb2_queue *q = vb->vb2_queue;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> void *mem_priv;
>> int plane;
>> int ret = -ENOMEM;
>> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>
>> mem_priv = call_ptr_memop(vb, alloc,
>> q->alloc_devs[plane] ? : q->dev,
>> - q->dma_attrs, size, dma_dir, q->gfp_flags);
>> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
>> if (IS_ERR_OR_NULL(mem_priv)) {
>> if (mem_priv)
>> ret = PTR_ERR(mem_priv);
>> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
>> void *mem_priv;
>> unsigned int plane;
>> int ret = 0;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> bool reacquired = vb->planes[0].mem_priv == NULL;
>>
>> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
>> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
>> mem_priv = call_ptr_memop(vb, get_userptr,
>> q->alloc_devs[plane] ? : q->dev,
>> planes[plane].m.userptr,
>> - planes[plane].length, dma_dir);
>> + planes[plane].length, q->dma_dir);
>> if (IS_ERR(mem_priv)) {
>> dprintk(1, "failed acquiring userspace memory for plane %d\n",
>> plane);
>> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>> void *mem_priv;
>> unsigned int plane;
>> int ret = 0;
>> - enum dma_data_direction dma_dir =
>> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> bool reacquired = vb->planes[0].mem_priv == NULL;
>>
>> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
>> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>> /* Acquire each plane's memory */
>> mem_priv = call_ptr_memop(vb, attach_dmabuf,
>> q->alloc_devs[plane] ? : q->dev,
>> - dbuf, planes[plane].length, dma_dir);
>> + dbuf, planes[plane].length, q->dma_dir);
>> if (IS_ERR(mem_priv)) {
>> dprintk(1, "failed to attach dmabuf\n");
>> ret = PTR_ERR(mem_priv);
>> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
>> if (q->buf_struct_size == 0)
>> q->buf_struct_size = sizeof(struct vb2_buffer);
>>
>> + if (q->bidirectional)
>> + q->dma_dir = DMA_BIDIRECTIONAL;
>> + else
>> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index cb97c224be73..ef9b64398c8c 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
>> * @dev: device to use for the default allocation context if the driver
>> * doesn't fill in the @alloc_devs array.
>> * @dma_attrs: DMA attributes to use for the DMA.
>> + * @bidirectional: when this flag is set the DMA direction for the buffers of
>> + * this queue will be overridden with DMA_BIDIRECTIONAL direction.
>> + * This is useful in cases where the hardware (firmware) writes to
>> + * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
>> + * buffer which is mapped for write (DMA_FROM_DEVICE) in order
>> + * to satisfy some internal hardware restrictions or adds a padding
>> + * needed by the processing algorithm. In case the DMA mapping is
>> + * not bidirectional but the hardware (firmware) trying to access
>> + * the buffer (in the opposite direction) this could lead to an
>> + * IOMMU protection faults.
>> * @fileio_read_once: report EOF after reading the first buffer
>> * @fileio_write_immediately: queue buffer after each write() call
>> * @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
>> @@ -465,6 +475,7 @@ struct vb2_buf_ops {
>> * Private elements (won't appear at the uAPI book):
>> * @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
>> * @memory: current memory type used
>> + * @dma_dir: DMA mapping direction.
>> * @bufs: videobuf buffer structures
>> * @num_buffers: number of allocated/used buffers
>> * @queued_list: list of buffers currently queued from userspace
>> @@ -495,6 +506,7 @@ struct vb2_queue {
>> unsigned int io_modes;
>> struct device *dev;
>> unsigned long dma_attrs;
>> + unsigned bidirectional:1;
>> unsigned fileio_read_once:1;
>> unsigned fileio_write_immediately:1;
>> unsigned allow_zero_bytesused:1;
>> @@ -516,6 +528,7 @@ struct vb2_queue {
>> /* private: internal use only */
>> struct mutex mmap_lock;
>> unsigned int memory;
>> + enum dma_data_direction dma_dir;
>> struct vb2_buffer *bufs[VB2_MAX_FRAME];
>> unsigned int num_buffers;
>>
>
> Best regards
>
This change is intended to give to the v4l2 drivers a choice to
change the default behavior of the v4l2-core DMA mapping direction
from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
Initially the issue with DMA mapping direction has been found in
Venus encoder driver where the hardware (firmware side) adds few
lines padding on bottom of the image buffer, and the consequence
is triggering of IOMMU protection faults.
This will help supporting venus encoder (and probably other drivers
in the future) which wants to map output type of buffers as
read/write.
Signed-off-by: Stanimir Varbanov <[email protected]>
---
v3: update V4L2 dma-sg/contig and vmalloc memory type ops with a
check for DMA_BIDIRECTIONAL.
v2: move dma_dir into private section.
drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
drivers/media/v4l2-core/videobuf2-dma-contig.c | 3 ++-
drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++--
drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 ++++--
include/media/videobuf2-core.h | 13 +++++++++++++
5 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 0924594989b4..cb115ba6a1d2 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
void *mem_priv;
int plane;
int ret = -ENOMEM;
@@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
- q->dma_attrs, size, dma_dir, q->gfp_flags);
+ q->dma_attrs, size, q->dma_dir, q->gfp_flags);
if (IS_ERR_OR_NULL(mem_priv)) {
if (mem_priv)
ret = PTR_ERR(mem_priv);
@@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
mem_priv = call_ptr_memop(vb, get_userptr,
q->alloc_devs[plane] ? : q->dev,
planes[plane].m.userptr,
- planes[plane].length, dma_dir);
+ planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed acquiring userspace memory for plane %d\n",
plane);
@@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
void *mem_priv;
unsigned int plane;
int ret = 0;
- enum dma_data_direction dma_dir =
- q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
bool reacquired = vb->planes[0].mem_priv == NULL;
memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
@@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(vb, attach_dmabuf,
q->alloc_devs[plane] ? : q->dev,
- dbuf, planes[plane].length, dma_dir);
+ dbuf, planes[plane].length, q->dma_dir);
if (IS_ERR(mem_priv)) {
dprintk(1, "failed to attach dmabuf\n");
ret = PTR_ERR(mem_priv);
@@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
if (q->buf_struct_size == 0)
q->buf_struct_size = sizeof(struct vb2_buffer);
+ if (q->bidirectional)
+ q->dma_dir = DMA_BIDIRECTIONAL;
+ else
+ q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
+
return 0;
}
EXPORT_SYMBOL_GPL(vb2_core_queue_init);
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 5b90a66b9e78..9f389f36566d 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -508,7 +508,8 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
buf->dma_dir = dma_dir;
offset = vaddr & ~PAGE_MASK;
- vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
+ vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
+ dma_dir == DMA_BIDIRECTIONAL);
if (IS_ERR(vec)) {
ret = PTR_ERR(vec);
goto fail_buf;
diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
index 54f33938d45b..6808231a6bdc 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
@@ -239,7 +239,8 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
buf->offset = vaddr & ~PAGE_MASK;
buf->size = size;
buf->dma_sgt = &buf->sg_table;
- vec = vb2_create_framevec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE);
+ vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
+ dma_dir == DMA_BIDIRECTIONAL);
if (IS_ERR(vec))
goto userptr_fail_pfnvec;
buf->vec = vec;
@@ -292,7 +293,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
vm_unmap_ram(buf->vaddr, buf->num_pages);
sg_free_table(buf->dma_sgt);
while (--i >= 0) {
- if (buf->dma_dir == DMA_FROM_DEVICE)
+ if (buf->dma_dir == DMA_FROM_DEVICE ||
+ buf->dma_dir == DMA_BIDIRECTIONAL)
set_page_dirty_lock(buf->pages[i]);
}
vb2_destroy_framevec(buf->vec);
diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
index 6bc130fe84f6..3a7c80cd1a17 100644
--- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
+++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
@@ -87,7 +87,8 @@ static void *vb2_vmalloc_get_userptr(struct device *dev, unsigned long vaddr,
buf->dma_dir = dma_dir;
offset = vaddr & ~PAGE_MASK;
buf->size = size;
- vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
+ vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
+ dma_dir == DMA_BIDIRECTIONAL);
if (IS_ERR(vec)) {
ret = PTR_ERR(vec);
goto fail_pfnvec_create;
@@ -137,7 +138,8 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
pages = frame_vector_pages(buf->vec);
if (vaddr)
vm_unmap_ram((void *)vaddr, n_pages);
- if (buf->dma_dir == DMA_FROM_DEVICE)
+ if (buf->dma_dir == DMA_FROM_DEVICE ||
+ buf->dma_dir == DMA_BIDIRECTIONAL)
for (i = 0; i < n_pages; i++)
set_page_dirty_lock(pages[i]);
} else {
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index cb97c224be73..ef9b64398c8c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,16 @@ struct vb2_buf_ops {
* @dev: device to use for the default allocation context if the driver
* doesn't fill in the @alloc_devs array.
* @dma_attrs: DMA attributes to use for the DMA.
+ * @bidirectional: when this flag is set the DMA direction for the buffers of
+ * this queue will be overridden with DMA_BIDIRECTIONAL direction.
+ * This is useful in cases where the hardware (firmware) writes to
+ * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
+ * buffer which is mapped for write (DMA_FROM_DEVICE) in order
+ * to satisfy some internal hardware restrictions or adds a padding
+ * needed by the processing algorithm. In case the DMA mapping is
+ * not bidirectional but the hardware (firmware) trying to access
+ * the buffer (in the opposite direction) this could lead to an
+ * IOMMU protection faults.
* @fileio_read_once: report EOF after reading the first buffer
* @fileio_write_immediately: queue buffer after each write() call
* @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
@@ -465,6 +475,7 @@ struct vb2_buf_ops {
* Private elements (won't appear at the uAPI book):
* @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
* @memory: current memory type used
+ * @dma_dir: DMA mapping direction.
* @bufs: videobuf buffer structures
* @num_buffers: number of allocated/used buffers
* @queued_list: list of buffers currently queued from userspace
@@ -495,6 +506,7 @@ struct vb2_queue {
unsigned int io_modes;
struct device *dev;
unsigned long dma_attrs;
+ unsigned bidirectional:1;
unsigned fileio_read_once:1;
unsigned fileio_write_immediately:1;
unsigned allow_zero_bytesused:1;
@@ -516,6 +528,7 @@ struct vb2_queue {
/* private: internal use only */
struct mutex mmap_lock;
unsigned int memory;
+ enum dma_data_direction dma_dir;
struct vb2_buffer *bufs[VB2_MAX_FRAME];
unsigned int num_buffers;
--
2.11.0
Hi,
On 08/21/2017 01:21 PM, Hans Verkuil wrote:
> On 08/21/2017 11:29 AM, Marek Szyprowski wrote:
>> Hi Stanimir,
>>
>> On 2017-08-21 11:09, Stanimir Varbanov wrote:
>>> This change is intended to give to the v4l2 drivers a choice to
>>> change the default behavior of the v4l2-core DMA mapping direction
>>> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
>>> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>>>
>>> Initially the issue with DMA mapping direction has been found in
>>> Venus encoder driver where the hardware (firmware side) adds few
>>> lines padding on bottom of the image buffer, and the consequence
>>> is triggering of IOMMU protection faults.
>>>
>>> This will help supporting venus encoder (and probably other drivers
>>> in the future) which wants to map output type of buffers as
>>> read/write.
>>>
>>> Signed-off-by: Stanimir Varbanov <[email protected]>
>>
>> This has been already discussed about a year ago, but it got lost in
>> meantime, probably due to lack of users. I hope that this time it
>> finally will get into vb2.
>>
>> For the reference, see https://patchwork.kernel.org/patch/9388163/
>
> Interesting.
>
> Stanimir, I like your implementation better than the macro in the old
> patch. But as it said there, videobuf2-dma-sg/contig/vmalloc.c have references
> to DMA_FROM_DEVICE that won't work with BIDIRECTIONAL, so those need
> to be adapted as well. I missed that when I reviewed your patch :-(
Thanks for catching this, I didn't thought too much about usrptr. Sent v3.
--
regards,
Stan
Hi Stanimir,
On 2017-08-21 13:34, Stanimir Varbanov wrote:
> This change is intended to give to the v4l2 drivers a choice to
> change the default behavior of the v4l2-core DMA mapping direction
> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>
> Initially the issue with DMA mapping direction has been found in
> Venus encoder driver where the hardware (firmware side) adds few
> lines padding on bottom of the image buffer, and the consequence
> is triggering of IOMMU protection faults.
>
> This will help supporting venus encoder (and probably other drivers
> in the future) which wants to map output type of buffers as
> read/write.
>
> Signed-off-by: Stanimir Varbanov <[email protected]>
Thanks for the patch.
Acked-by: Marek Szyprowski <[email protected]>
While touching this, I would love to unify set_page_dirty_lock()
related code in videobuf2-dc, videobuf2-sg and videobuf2-vmalloc.
IMHO the pattern used in videobuf2-vmalloc should be copied to
videobuf2-sg (currently checks for dma_dir for every single page)
and videobuf2-dc (currently it lacks any checks and calls
set_page_dirty_lock() unconditionally). If you have a little bit
of spare time, please prepare a separate patch for the above
mentioned fix.
> ---
> v3: update V4L2 dma-sg/contig and vmalloc memory type ops with a
> check for DMA_BIDIRECTIONAL.
> v2: move dma_dir into private section.
>
> drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++---------
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 3 ++-
> drivers/media/v4l2-core/videobuf2-dma-sg.c | 6 ++++--
> drivers/media/v4l2-core/videobuf2-vmalloc.c | 6 ++++--
> include/media/videobuf2-core.h | 13 +++++++++++++
> 5 files changed, 31 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 0924594989b4..cb115ba6a1d2 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -194,8 +194,6 @@ static void __enqueue_in_driver(struct vb2_buffer *vb);
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> void *mem_priv;
> int plane;
> int ret = -ENOMEM;
> @@ -209,7 +207,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>
> mem_priv = call_ptr_memop(vb, alloc,
> q->alloc_devs[plane] ? : q->dev,
> - q->dma_attrs, size, dma_dir, q->gfp_flags);
> + q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> if (IS_ERR_OR_NULL(mem_priv)) {
> if (mem_priv)
> ret = PTR_ERR(mem_priv);
> @@ -978,8 +976,6 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1030,7 +1026,7 @@ static int __prepare_userptr(struct vb2_buffer *vb, const void *pb)
> mem_priv = call_ptr_memop(vb, get_userptr,
> q->alloc_devs[plane] ? : q->dev,
> planes[plane].m.userptr,
> - planes[plane].length, dma_dir);
> + planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed acquiring userspace memory for plane %d\n",
> plane);
> @@ -1096,8 +1092,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - enum dma_data_direction dma_dir =
> - q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> @@ -1156,7 +1150,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(vb, attach_dmabuf,
> q->alloc_devs[plane] ? : q->dev,
> - dbuf, planes[plane].length, dma_dir);
> + dbuf, planes[plane].length, q->dma_dir);
> if (IS_ERR(mem_priv)) {
> dprintk(1, "failed to attach dmabuf\n");
> ret = PTR_ERR(mem_priv);
> @@ -2003,6 +1997,11 @@ int vb2_core_queue_init(struct vb2_queue *q)
> if (q->buf_struct_size == 0)
> q->buf_struct_size = sizeof(struct vb2_buffer);
>
> + if (q->bidirectional)
> + q->dma_dir = DMA_BIDIRECTIONAL;
> + else
> + q->dma_dir = q->is_output ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(vb2_core_queue_init);
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> index 5b90a66b9e78..9f389f36566d 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -508,7 +508,8 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr,
> buf->dma_dir = dma_dir;
>
> offset = vaddr & ~PAGE_MASK;
> - vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_buf;
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> index 54f33938d45b..6808231a6bdc 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c
> @@ -239,7 +239,8 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,
> buf->offset = vaddr & ~PAGE_MASK;
> buf->size = size;
> buf->dma_sgt = &buf->sg_table;
> - vec = vb2_create_framevec(vaddr, size, buf->dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec))
> goto userptr_fail_pfnvec;
> buf->vec = vec;
> @@ -292,7 +293,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> vm_unmap_ram(buf->vaddr, buf->num_pages);
> sg_free_table(buf->dma_sgt);
> while (--i >= 0) {
> - if (buf->dma_dir == DMA_FROM_DEVICE)
> + if (buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL)
> set_page_dirty_lock(buf->pages[i]);
> }
> vb2_destroy_framevec(buf->vec);
> diff --git a/drivers/media/v4l2-core/videobuf2-vmalloc.c b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> index 6bc130fe84f6..3a7c80cd1a17 100644
> --- a/drivers/media/v4l2-core/videobuf2-vmalloc.c
> +++ b/drivers/media/v4l2-core/videobuf2-vmalloc.c
> @@ -87,7 +87,8 @@ static void *vb2_vmalloc_get_userptr(struct device *dev, unsigned long vaddr,
> buf->dma_dir = dma_dir;
> offset = vaddr & ~PAGE_MASK;
> buf->size = size;
> - vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE);
> + vec = vb2_create_framevec(vaddr, size, dma_dir == DMA_FROM_DEVICE ||
> + dma_dir == DMA_BIDIRECTIONAL);
> if (IS_ERR(vec)) {
> ret = PTR_ERR(vec);
> goto fail_pfnvec_create;
> @@ -137,7 +138,8 @@ static void vb2_vmalloc_put_userptr(void *buf_priv)
> pages = frame_vector_pages(buf->vec);
> if (vaddr)
> vm_unmap_ram((void *)vaddr, n_pages);
> - if (buf->dma_dir == DMA_FROM_DEVICE)
> + if (buf->dma_dir == DMA_FROM_DEVICE ||
> + buf->dma_dir == DMA_BIDIRECTIONAL)
> for (i = 0; i < n_pages; i++)
> set_page_dirty_lock(pages[i]);
> } else {
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index cb97c224be73..ef9b64398c8c 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -427,6 +427,16 @@ struct vb2_buf_ops {
> * @dev: device to use for the default allocation context if the driver
> * doesn't fill in the @alloc_devs array.
> * @dma_attrs: DMA attributes to use for the DMA.
> + * @bidirectional: when this flag is set the DMA direction for the buffers of
> + * this queue will be overridden with DMA_BIDIRECTIONAL direction.
> + * This is useful in cases where the hardware (firmware) writes to
> + * a buffer which is mapped as read (DMA_TO_DEVICE), or reads from
> + * buffer which is mapped for write (DMA_FROM_DEVICE) in order
> + * to satisfy some internal hardware restrictions or adds a padding
> + * needed by the processing algorithm. In case the DMA mapping is
> + * not bidirectional but the hardware (firmware) trying to access
> + * the buffer (in the opposite direction) this could lead to an
> + * IOMMU protection faults.
> * @fileio_read_once: report EOF after reading the first buffer
> * @fileio_write_immediately: queue buffer after each write() call
> * @allow_zero_bytesused: allow bytesused == 0 to be passed to the driver
> @@ -465,6 +475,7 @@ struct vb2_buf_ops {
> * Private elements (won't appear at the uAPI book):
> * @mmap_lock: private mutex used when buffers are allocated/freed/mmapped
> * @memory: current memory type used
> + * @dma_dir: DMA mapping direction.
> * @bufs: videobuf buffer structures
> * @num_buffers: number of allocated/used buffers
> * @queued_list: list of buffers currently queued from userspace
> @@ -495,6 +506,7 @@ struct vb2_queue {
> unsigned int io_modes;
> struct device *dev;
> unsigned long dma_attrs;
> + unsigned bidirectional:1;
> unsigned fileio_read_once:1;
> unsigned fileio_write_immediately:1;
> unsigned allow_zero_bytesused:1;
> @@ -516,6 +528,7 @@ struct vb2_queue {
> /* private: internal use only */
> struct mutex mmap_lock;
> unsigned int memory;
> + enum dma_data_direction dma_dir;
> struct vb2_buffer *bufs[VB2_MAX_FRAME];
> unsigned int num_buffers;
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek,
On 08/21/2017 03:21 PM, Marek Szyprowski wrote:
> Hi Stanimir,
>
> On 2017-08-21 13:34, Stanimir Varbanov wrote:
>> This change is intended to give to the v4l2 drivers a choice to
>> change the default behavior of the v4l2-core DMA mapping direction
>> from DMA_TO/FROM_DEVICE (depending on the buffer type CAPTURE or
>> OUTPUT) to DMA_BIDIRECTIONAL during queue_init time.
>>
>> Initially the issue with DMA mapping direction has been found in
>> Venus encoder driver where the hardware (firmware side) adds few
>> lines padding on bottom of the image buffer, and the consequence
>> is triggering of IOMMU protection faults.
>>
>> This will help supporting venus encoder (and probably other drivers
>> in the future) which wants to map output type of buffers as
>> read/write.
>>
>> Signed-off-by: Stanimir Varbanov <[email protected]>
>
> Thanks for the patch.
>
> Acked-by: Marek Szyprowski <[email protected]>
Thanks!
>
> While touching this, I would love to unify set_page_dirty_lock()
> related code in videobuf2-dc, videobuf2-sg and videobuf2-vmalloc.
>
> IMHO the pattern used in videobuf2-vmalloc should be copied to
> videobuf2-sg (currently checks for dma_dir for every single page)
> and videobuf2-dc (currently it lacks any checks and calls
> set_page_dirty_lock() unconditionally). If you have a little bit
> of spare time, please prepare a separate patch for the above
> mentioned fix.
Sure, I'll unify set_page_dirty_lock invocations in separate patch.
--
regards,
Stan