* [PATCH] drm/amdgpu: fix possible null-pointer dereference in amdgpu_ttm_tt_unpopulate()
From: Tuo Li @ 2021-07-31 8:13 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
sumit.semwal, airlied, Felix.Kuehling, Oak.Zeng, nirmoy.das,
tzimmermann, Philip.Yang
Cc: amd-gfx, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
baijiaju1990, Tuo Li, TOTE Robot
The variable ttm is assigned to the variable gtt, and the variable gtt
is checked in:
if (gtt && gtt->userptr)
This indicates that both ttm and gtt can be NULL.
If so, a null-pointer dereference will occur:
if (ttm->page_flags & TTM_PAGE_FLAG_SG)
Also, some null-pointer dereferences will occur in the function
ttm_pool_free() which is called in:
return ttm_pool_free(&adev->mman.bdev.pool, ttm);
To fix these possible null-pointer dereferences, the function returns
when ttm is NULL.
Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3a55f08e00e1..0216ca085f11 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1146,7 +1146,10 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_device *bdev,
struct amdgpu_ttm_tt *gtt = (void *)ttm;
struct amdgpu_device *adev;
- if (gtt && gtt->userptr) {
+ if (ttm == NULL)
+ return;
+
+ if (gtt->userptr) {
amdgpu_ttm_tt_set_user_pages(ttm, NULL);
kfree(ttm->sg);
ttm->sg = NULL;
--
2.25.1
^ permalink raw reply related
* [PATCH] drm/amdgpu: fix possible null-pointer dereference in amdgpu_ttm_tt_populate()
From: Tuo Li @ 2021-07-31 8:04 UTC (permalink / raw)
To: alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
sumit.semwal, airlied, Felix.Kuehling, Oak.Zeng, nirmoy.das,
tzimmermann, Philip.Yang
Cc: amd-gfx, dri-devel, linux-kernel, linux-media, linaro-mm-sig,
baijiaju1990, Tuo Li, TOTE Robot
The variable ttm is assigned to the variable gtt, and the variable gtt
is checked in:
if (gtt && gtt->userptr)
This indicates that both ttm and gtt can be NULL.
If so, a null-pointer dereference will occur:
if (ttm->page_flags & TTM_PAGE_FLAG_SG)
Also, some null-pointer dereferences will occur in the function
ttm_pool_alloc() which is called in:
return ttm_pool_alloc(&adev->mman.bdev.pool, ttm, ctx);
To fix these possible null-pointer dereferences, the function returns
-EINVAL when ttm is NULL.
Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 3a55f08e00e1..80440f799c09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1120,8 +1120,11 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev,
struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
struct amdgpu_ttm_tt *gtt = (void *)ttm;
+ if (ttm == NULL)
+ return -EINVAL;
+
/* user pages are bound by amdgpu_ttm_tt_pin_userptr() */
- if (gtt && gtt->userptr) {
+ if (gtt->userptr) {
ttm->sg = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
if (!ttm->sg)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related
* [PATCH] scsi: csiostor: fix possible null-pointer dereference in csio_eh_lun_reset_handler()
From: Tuo Li @ 2021-07-31 7:51 UTC (permalink / raw)
To: jejb, martin.petersen, sumit.semwal, christian.koenig, colin.king,
jiapeng.chong
Cc: linux-scsi, linux-kernel, linux-media, dri-devel, linaro-mm-sig,
baijiaju1990, Tuo Li, TOTE Robot
The variable rn is checked in:
if (!rn)
If rn is NULL, the program goes to the label fail:
fail:
CSIO_INC_STATS(rn, n_lun_rst_fail);
However, rn is dereferenced in this macro:
#define CSIO_INC_STATS(elem, val) ((elem)->stats.val++)
To fix this possible null-pointer dereference, the function returns
FAILED directly if rn is NULL.
Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
drivers/scsi/csiostor/csio_scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/csiostor/csio_scsi.c b/drivers/scsi/csiostor/csio_scsi.c
index 56b9ad0a1ca0..df0bf8348860 100644
--- a/drivers/scsi/csiostor/csio_scsi.c
+++ b/drivers/scsi/csiostor/csio_scsi.c
@@ -2070,7 +2070,7 @@ csio_eh_lun_reset_handler(struct scsi_cmnd *cmnd)
struct csio_scsi_level_data sld;
if (!rn)
- goto fail;
+ return FAILED;
csio_dbg(hw, "Request to reset LUN:%llu (ssni:0x%x tgtid:%d)\n",
cmnd->device->lun, rn->flowid, rn->scsi_id);
--
2.25.1
^ permalink raw reply related
* [PATCH] media: isl6421: fix possible uninitialized-variable access in isl6421_set_voltage()
From: Tuo Li @ 2021-07-31 7:38 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, linux-kernel, baijiaju1990, Tuo Li, TOTE Robot
A memory block is allocated through kmalloc(), and its return value is
assigned to the pointer isl6421. Then isl6421 is assigned to the
varialbe fe->sec_priv. The function isl6421_set_voltage() is called with
the argument fe. In this function, fe->sec_priv is assigned to isl6421.
Thus the pointer isl6421 in the function isl6421_attach() and the function
isl6421_set_voltage() point to the same memory. However, isl6421->is_off
is not initialized but it is accessed at line 75:
if (isl6421->is_off && !is_off)
To fix this possible uninitialized-variable access, isl6421->is_off is
initialized to false in the function isl6421_attach().
Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
drivers/media/dvb-frontends/isl6421.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/dvb-frontends/isl6421.c b/drivers/media/dvb-frontends/isl6421.c
index 43b0dfc6f453..ea101f66ea88 100644
--- a/drivers/media/dvb-frontends/isl6421.c
+++ b/drivers/media/dvb-frontends/isl6421.c
@@ -185,6 +185,7 @@ struct dvb_frontend *isl6421_attach(struct dvb_frontend *fe, struct i2c_adapter
isl6421->config = ISL6421_ISEL1;
isl6421->i2c = i2c;
isl6421->i2c_addr = i2c_addr;
+ isl6421->is_off = false;
fe->sec_priv = isl6421;
/* bits which should be forced to '1' */
--
2.25.1
^ permalink raw reply related
* [PATCH] media: mxl111sf: change mutex_init() location
From: Pavel Skripkin @ 2021-07-30 21:38 UTC (permalink / raw)
To: mkrufky, mchehab, crope
Cc: linux-media, linux-kernel, Pavel Skripkin,
syzbot+5ca0bf339f13c4243001
Syzbot reported, that mxl111sf_ctrl_msg() uses uninitialized
mutex. The problem was in wrong mutex_init() location.
Previous mutex_init(&state->msg_lock) call was in ->init() function, but
dvb_usbv2_init() has this order of calls:
dvb_usbv2_init()
dvb_usbv2_adapter_init()
dvb_usbv2_adapter_frontend_init()
props->frontend_attach()
props->init()
Since mxl111sf_frontend_attach_atsc_mh() calls mxl111sf_ctrl_msg()
internally we need to initialize state->msg_lock in it to make lockdep
happy.
Reported-and-tested-by: syzbot+5ca0bf339f13c4243001@syzkaller.appspotmail.com
Fixes: 8572211842af ("[media] mxl111sf: convert to new DVB USB")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
drivers/media/usb/dvb-usb-v2/mxl111sf.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/dvb-usb-v2/mxl111sf.c b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
index 7865fa0a8295..2e5663ffa7ce 100644
--- a/drivers/media/usb/dvb-usb-v2/mxl111sf.c
+++ b/drivers/media/usb/dvb-usb-v2/mxl111sf.c
@@ -931,8 +931,6 @@ static int mxl111sf_init(struct dvb_usb_device *d)
.len = sizeof(eeprom), .buf = eeprom },
};
- mutex_init(&state->msg_lock);
-
ret = get_chip_info(state);
if (mxl_fail(ret))
pr_err("failed to get chip info during probe");
@@ -979,8 +977,12 @@ static int mxl111sf_frontend_attach_mh(struct dvb_usb_adapter *adap)
static int mxl111sf_frontend_attach_atsc_mh(struct dvb_usb_adapter *adap)
{
int ret;
+ struct mxl111sf_state *state = d_to_priv(adap_to_d(adap));
+
pr_debug("%s\n", __func__);
+ mutex_init(&state->msg_lock);
+
ret = mxl111sf_lgdt3305_frontend_attach(adap, 0);
if (ret < 0)
return ret;
--
2.32.0
^ permalink raw reply related
* Re: [PATCH 3/3] dma-buf: nuke SW_SYNC debugfs files
From: Hridya Valsaraju @ 2021-07-30 20:46 UTC (permalink / raw)
To: John Stultz
Cc: Daniel Vetter, Christian König, Gustavo Padovan, dri-devel,
moderated list:DMA BUFFER SHARING FRAMEWORK, linux-media,
Alistair Delva, Sandeep Patil
In-Reply-To: <CALAqxLVN7RVz3+z1ZvkRHeb2=Y4KbpbTOw-8St0D+Lzt5U-cFw@mail.gmail.com>
On Thu, Jul 29, 2021 at 9:52 PM John Stultz <john.stultz@linaro.org> wrote:
>
> On Thu, Jul 29, 2021 at 12:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Thu, Jul 29, 2021 at 09:03:30AM +0200, Christian König wrote:
> > > As we now knew controlling dma_fence synchronization from userspace is
> > > extremely dangerous and can not only deadlock drivers but trivially also the
> > > whole kernel memory management.
> > >
> > > Entirely remove this option. We now have in kernel unit tests to exercise the
> > > dma_fence framework and it's containers.
> > >
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> >
> > There's also igts for this, and Android heavily uses this. So I'm not sure
> > we can just nuke it.
>
> Eeeeh... I don't think that's actually the case anymore. As of
> android12-5.10 CONFIG_SW_SYNC is not turned on.
> Further, Android is disabling debugfs in their kernels as it exposes
> too much to userland.
>
> That said, there still are some references to it:
> https://cs.android.com/android/platform/superproject/+/master:system/core/libsync/sync.c;l=416
Hello,
Like John mentioned, CONFIG_SW_SYNC is not turned on for the *-5.4 and
newer branches in the Android Common Kernel. The references in AOSP
are only in place to support devices with older kernels upgrading to
newer Android versions.
Regards,
Hridya
>
> But it looks like the actual users are only kselftest and igt?
>
> Adding Alistair, Hridya and Sandeep in case they have more context.
>
> thanks
> -john
^ permalink raw reply
* [PATCH v2] media: meson-ge2d: Fix rotation parameter changes detection in 'ge2d_s_ctrl()'
From: Christophe JAILLET @ 2021-07-30 19:35 UTC (permalink / raw)
To: narmstrong, mchehab, khilman, jbrunet, martin.blumenstingl,
hverkuil-cisco
Cc: linux-media, linux-amlogic, linux-arm-kernel, linux-kernel,
kernel-janitors, Christophe JAILLET
There is likely a typo here. To be consistent, we should compare
'fmt.height' with 'ctx->out.pix_fmt.height', not 'ctx->out.pix_fmt.width'.
Instead of fixing the test, just remove it and copy 'fmt' unconditionally.
Fixes: 59a635327ca7 ("media: meson: Add M2M driver for the Amlogic GE2D Accelerator Unit")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
---
v2: axe the tests. The structure is small, and it is likely that it was
already copied with the broken test.
---
drivers/media/platform/meson/ge2d/ge2d.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/media/platform/meson/ge2d/ge2d.c b/drivers/media/platform/meson/ge2d/ge2d.c
index a1393fefa8ae..9b1e973e78da 100644
--- a/drivers/media/platform/meson/ge2d/ge2d.c
+++ b/drivers/media/platform/meson/ge2d/ge2d.c
@@ -779,11 +779,7 @@ static int ge2d_s_ctrl(struct v4l2_ctrl *ctrl)
* If the rotation parameter changes the OUTPUT frames
* parameters, take them in account
*/
- if (fmt.width != ctx->out.pix_fmt.width ||
- fmt.height != ctx->out.pix_fmt.width ||
- fmt.bytesperline > ctx->out.pix_fmt.bytesperline ||
- fmt.sizeimage > ctx->out.pix_fmt.sizeimage)
- ctx->out.pix_fmt = fmt;
+ ctx->out.pix_fmt = fmt;
break;
}
--
2.30.2
^ permalink raw reply related
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Laurent Pinchart @ 2021-07-30 16:56 UTC (permalink / raw)
To: Nicolas Dufresne
Cc: Dafna Hirschfeld, linux-media, linux-renesas-soc, Helen Koike,
Shuah Khan
In-Reply-To: <5cde7f090939ac43588c682f2626f07ca9648dd7.camel@ndufresne.ca>
Hi Nicolas,
On Fri, Jul 30, 2021 at 11:59:27AM -0400, Nicolas Dufresne wrote:
> Le vendredi 30 juillet 2021 à 17:15 +0300, Laurent Pinchart a écrit :
> > On Fri, Jul 30, 2021 at 04:08:11PM +0200, Dafna Hirschfeld wrote:
> > > On 30.07.21 15:11, Laurent Pinchart wrote:
> > > > On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
> > > > > On 30.07.21 02:19, Laurent Pinchart wrote:
> > > > > > The vimc driver is used for testing purpose, and some test use cases
> > > > > > involve sharing buffers with a consumer device. Consumers often require
> > > > > > DMA contiguous memory, which vimc doesn't currently support. This leads
> > > > > > in the best case to usage of bounce buffers, which is very slow, and in
> > > > > > the worst case in a complete failure.
> > > > > >
> > > > > > Add support for the dma-contig allocator in vimc to support those use
> > > > > > cases properly. The allocator is selected through a new "allocator"
> > > > > > module parameter, which defaults to vmalloc.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > ---
> > > > > > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > > > > > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > > > > > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > > > > > 3 files changed, 19 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > > index 5e9fd902cd37..92b69a6529fb 100644
> > > > > > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > > @@ -7,6 +7,7 @@
> > > > > >
> > > > > > #include <media/v4l2-ioctl.h>
> > > > > > #include <media/videobuf2-core.h>
> > > > > > +#include <media/videobuf2-dma-contig.h>
> > > > > > #include <media/videobuf2-vmalloc.h>
> > > > > >
> > > > > > #include "vimc-common.h"
> > > > > > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > > > > > /* Initialize the vb2 queue */
> > > > > > q = &vcap->queue;
> > > > > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > > > > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > > > > > + q->io_modes = VB2_MMAP | VB2_DMABUF;
> > > > >
> > > > > maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
> > > >
> > > > Why so ? vb2-vmalloc can import dma-bufs.
> > >
> > > oh, I meant that exporting should not be supported.
> > > I see that vimc set ".vidioc_expbuf = vb2_ioctl_expbuf", maybe remove that if allocator is vmalloc?
> >
> > If the importer support non-contiguous buffers, vb2-vmalloc can be used
> > as an exporter. I've successfully used this to test sharing buffers
> > between vimc in vmalloc mode and the R-Car H3 display driver with an
> > IOMMU.
>
> Having an IOMMU is not sufficient, as this is shown with Intel DRM. The DRM
> driver needs to support CPU cache. Note that in GStreamer this is used a lot
> from UVC camera to GL (but it breaks, corrupted images, with kmssink).
That depends on the platform. When the sink device isn't cache-coherent,
it needs to perform a cache sync operation. V4L2 handles this
automatically for codecs (technically speaking for cameras as well, but
cameras are rarely sinks :-)). In DRM/KMS, there's still quite a bit of
work to do. It's not an issue in vimc in any case.
> > > > > > + if (vimc_allocator != 1)
> > > > >
> > > > > maybe define a macro instead of `1` ?
> > > >
> > > > Good idea.
> > > >
> > > > > > + q->io_modes |= VB2_USERPTR;
> > > > > > q->drv_priv = vcap;
> > > > > > q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> > > > > > q->ops = &vimc_cap_qops;
> > > > > > - q->mem_ops = &vb2_vmalloc_memops;
> > > > > > + q->mem_ops = vimc_allocator == 1
> > > > > > + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
> > > > > > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > > > q->min_buffers_needed = 2;
> > > > > > q->lock = &vcap->lock;
> > > > > > + q->dev = v4l2_dev->dev;
> > > > > >
> > > > > > ret = vb2_queue_init(q);
> > > > > > if (ret) {
> > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > > index a289434e75ba..b77939123501 100644
> > > > > > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > > @@ -35,6 +35,8 @@
> > > > > >
> > > > > > #define VIMC_PIX_FMT_MAX_CODES 8
> > > > > >
> > > > > > +extern unsigned int vimc_allocator;
> > > > > > +
> > > > > > /**
> > > > > > * vimc_colorimetry_clamp - Adjust colorimetry parameters
> > > > > > *
> > > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > > index 4b0ae6f51d76..7badcecb7aed 100644
> > > > > > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > > @@ -5,6 +5,7 @@
> > > > > > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> > > > > > */
> > > > > >
> > > > > > +#include <linux/dma-mapping.h>
> > > > > > #include <linux/font.h>
> > > > > > #include <linux/init.h>
> > > > > > #include <linux/module.h>
> > > > > > @@ -15,6 +16,12 @@
> > > > > >
> > > > > > #include "vimc-common.h"
> > > > > >
> > > > > > +unsigned int vimc_allocator;
> > > > > > +module_param_named(allocator, vimc_allocator, uint, 0444);
> > > > > > +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> > > > > > + "\t\t 0 == vmalloc\n"
> > > > > > + "\t\t 1 == dma-contig");
> > > > > > +
> > > > >
> > > > > There is a section 'Module options' in vimc.rst. So a doc should be added there.
> > > >
> > > > OK, I'll update that.
> > > >
> > > > > > #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> > > > > >
> > > > > > #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > > > > > @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
> > > > > >
> > > > > > tpg_set_font(font->data);
> > > > > >
> > > > > > + if (vimc_allocator == 1)
> > > > > > + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > > > > +
> > > > > > vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> > > > > > if (!vimc)
> > > > > > return -ENOMEM;
> > > > > >
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Laurent Pinchart @ 2021-07-30 16:50 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Dafna Hirschfeld, linux-media, Linux-Renesas, Helen Koike,
Shuah Khan
In-Reply-To: <CAAEAJfAv_yVUFQm7NYas46nXUQpBS1=3iuiJC+-bCTm5+AJCKg@mail.gmail.com>
Hi Ezequiel,
On Fri, Jul 30, 2021 at 01:13:20PM -0300, Ezequiel Garcia wrote:
> On Fri, 30 Jul 2021 at 09:35, Dafna Hirschfeld wrote:
> > On 30.07.21 02:19, Laurent Pinchart wrote:
> > > The vimc driver is used for testing purpose, and some test use cases
> > > involve sharing buffers with a consumer device. Consumers often require
> > > DMA contiguous memory, which vimc doesn't currently support. This leads
> > > in the best case to usage of bounce buffers, which is very slow, and in
> > > the worst case in a complete failure.
> > >
> > > Add support for the dma-contig allocator in vimc to support those use
> > > cases properly. The allocator is selected through a new "allocator"
> > > module parameter, which defaults to vmalloc.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > > 3 files changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > index 5e9fd902cd37..92b69a6529fb 100644
> > > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > @@ -7,6 +7,7 @@
> > >
> > > #include <media/v4l2-ioctl.h>
> > > #include <media/videobuf2-core.h>
> > > +#include <media/videobuf2-dma-contig.h>
> > > #include <media/videobuf2-vmalloc.h>
> > >
> > > #include "vimc-common.h"
> > > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > > /* Initialize the vb2 queue */
> > > q = &vcap->queue;
> > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > > + q->io_modes = VB2_MMAP | VB2_DMABUF;
> >
> > maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
> >
> > > + if (vimc_allocator != 1)
> >
> > maybe define a macro instead of `1` ?
>
> This is maybe an overkill, but you can make the parameter accept strings
> instead of integers, which makes it "modprobe vimc allocator=vmalloc",
> and improves a bit user-friendlyness.
>
> See drivers/media/pci/tw686x/tw686x-core.c.
>
> For a test driver, it is worth the trouble, maybe?
I copied the values from vivid, which uses an array of integers. As vimc
needs a single parameter only, a string could make more sense. I'll
submit a v3 with a string if there's a consensus for that.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Ezequiel Garcia @ 2021-07-30 16:13 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: Laurent Pinchart, linux-media, Linux-Renesas, Helen Koike,
Shuah Khan
In-Reply-To: <40a1fed8-456e-97c5-9aa7-715a4a4c816b@collabora.com>
Hi Laurent, Dafna,
On Fri, 30 Jul 2021 at 09:35, Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi
>
> On 30.07.21 02:19, Laurent Pinchart wrote:
> > The vimc driver is used for testing purpose, and some test use cases
> > involve sharing buffers with a consumer device. Consumers often require
> > DMA contiguous memory, which vimc doesn't currently support. This leads
> > in the best case to usage of bounce buffers, which is very slow, and in
> > the worst case in a complete failure.
> >
> > Add support for the dma-contig allocator in vimc to support those use
> > cases properly. The allocator is selected through a new "allocator"
> > module parameter, which defaults to vmalloc.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > 3 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index 5e9fd902cd37..92b69a6529fb 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -7,6 +7,7 @@
> >
> > #include <media/v4l2-ioctl.h>
> > #include <media/videobuf2-core.h>
> > +#include <media/videobuf2-dma-contig.h>
> > #include <media/videobuf2-vmalloc.h>
> >
> > #include "vimc-common.h"
> > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > /* Initialize the vb2 queue */
> > q = &vcap->queue;
> > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > + q->io_modes = VB2_MMAP | VB2_DMABUF;
>
> maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
>
> > + if (vimc_allocator != 1)
>
> maybe define a macro instead of `1` ?
>
This is maybe an overkill, but you can make the parameter accept strings
instead of integers, which makes it "modprobe vimc allocator=vmalloc",
and improves a bit user-friendlyness.
See drivers/media/pci/tw686x/tw686x-core.c.
For a test driver, it is worth the trouble, maybe?
Thanks,
Ezequiel
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Nicolas Dufresne @ 2021-07-30 15:59 UTC (permalink / raw)
To: Laurent Pinchart, Dafna Hirschfeld
Cc: linux-media, linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <YQQJhjOtPb10+olI@pendragon.ideasonboard.com>
Le vendredi 30 juillet 2021 à 17:15 +0300, Laurent Pinchart a écrit :
> Hi Dafna,
>
> On Fri, Jul 30, 2021 at 04:08:11PM +0200, Dafna Hirschfeld wrote:
> > On 30.07.21 15:11, Laurent Pinchart wrote:
> > > On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
> > > > On 30.07.21 02:19, Laurent Pinchart wrote:
> > > > > The vimc driver is used for testing purpose, and some test use cases
> > > > > involve sharing buffers with a consumer device. Consumers often require
> > > > > DMA contiguous memory, which vimc doesn't currently support. This leads
> > > > > in the best case to usage of bounce buffers, which is very slow, and in
> > > > > the worst case in a complete failure.
> > > > >
> > > > > Add support for the dma-contig allocator in vimc to support those use
> > > > > cases properly. The allocator is selected through a new "allocator"
> > > > > module parameter, which defaults to vmalloc.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > ---
> > > > > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > > > > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > > > > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > > > > 3 files changed, 19 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > index 5e9fd902cd37..92b69a6529fb 100644
> > > > > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > > > @@ -7,6 +7,7 @@
> > > > >
> > > > > #include <media/v4l2-ioctl.h>
> > > > > #include <media/videobuf2-core.h>
> > > > > +#include <media/videobuf2-dma-contig.h>
> > > > > #include <media/videobuf2-vmalloc.h>
> > > > >
> > > > > #include "vimc-common.h"
> > > > > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > > > > /* Initialize the vb2 queue */
> > > > > q = &vcap->queue;
> > > > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > > > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > > > > + q->io_modes = VB2_MMAP | VB2_DMABUF;
> > > >
> > > > maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
> > >
> > > Why so ? vb2-vmalloc can import dma-bufs.
> >
> > oh, I meant that exporting should not be supported.
> > I see that vimc set ".vidioc_expbuf = vb2_ioctl_expbuf", maybe remove that if allocator is vmalloc?
>
> If the importer support non-contiguous buffers, vb2-vmalloc can be used
> as an exporter. I've successfully used this to test sharing buffers
> between vimc in vmalloc mode and the R-Car H3 display driver with an
> IOMMU.
Having an IOMMU is not sufficient, as this is shown with Intel DRM. The DRM
driver needs to support CPU cache. Note that in GStreamer this is used a lot
from UVC camera to GL (but it breaks, corrupted images, with kmssink).
>
> > > > > + if (vimc_allocator != 1)
> > > >
> > > > maybe define a macro instead of `1` ?
> > >
> > > Good idea.
> > >
> > > > > + q->io_modes |= VB2_USERPTR;
> > > > > q->drv_priv = vcap;
> > > > > q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> > > > > q->ops = &vimc_cap_qops;
> > > > > - q->mem_ops = &vb2_vmalloc_memops;
> > > > > + q->mem_ops = vimc_allocator == 1
> > > > > + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
> > > > > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > > q->min_buffers_needed = 2;
> > > > > q->lock = &vcap->lock;
> > > > > + q->dev = v4l2_dev->dev;
> > > > >
> > > > > ret = vb2_queue_init(q);
> > > > > if (ret) {
> > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > index a289434e75ba..b77939123501 100644
> > > > > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > > > > @@ -35,6 +35,8 @@
> > > > >
> > > > > #define VIMC_PIX_FMT_MAX_CODES 8
> > > > >
> > > > > +extern unsigned int vimc_allocator;
> > > > > +
> > > > > /**
> > > > > * vimc_colorimetry_clamp - Adjust colorimetry parameters
> > > > > *
> > > > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > index 4b0ae6f51d76..7badcecb7aed 100644
> > > > > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > > > > @@ -5,6 +5,7 @@
> > > > > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> > > > > */
> > > > >
> > > > > +#include <linux/dma-mapping.h>
> > > > > #include <linux/font.h>
> > > > > #include <linux/init.h>
> > > > > #include <linux/module.h>
> > > > > @@ -15,6 +16,12 @@
> > > > >
> > > > > #include "vimc-common.h"
> > > > >
> > > > > +unsigned int vimc_allocator;
> > > > > +module_param_named(allocator, vimc_allocator, uint, 0444);
> > > > > +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> > > > > + "\t\t 0 == vmalloc\n"
> > > > > + "\t\t 1 == dma-contig");
> > > > > +
> > > >
> > > > There is a section 'Module options' in vimc.rst. So a doc should be added there.
> > >
> > > OK, I'll update that.
> > >
> > > > > #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> > > > >
> > > > > #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > > > > @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
> > > > >
> > > > > tpg_set_font(font->data);
> > > > >
> > > > > + if (vimc_allocator == 1)
> > > > > + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > > > +
> > > > > vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> > > > > if (!vimc)
> > > > > return -ENOMEM;
> > > > >
>
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Nicolas Dufresne @ 2021-07-30 15:56 UTC (permalink / raw)
To: Laurent Pinchart, Dafna Hirschfeld
Cc: linux-media, linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <YQP6aILfBDLhSFUt@pendragon.ideasonboard.com>
Le vendredi 30 juillet 2021 à 16:11 +0300, Laurent Pinchart a écrit :
> Hi Dafna,
>
> On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
> > On 30.07.21 02:19, Laurent Pinchart wrote:
> > > The vimc driver is used for testing purpose, and some test use cases
> > > involve sharing buffers with a consumer device. Consumers often require
> > > DMA contiguous memory, which vimc doesn't currently support. This leads
> > > in the best case to usage of bounce buffers, which is very slow, and in
> > > the worst case in a complete failure.
> > >
> > > Add support for the dma-contig allocator in vimc to support those use
> > > cases properly. The allocator is selected through a new "allocator"
> > > module parameter, which defaults to vmalloc.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > > 3 files changed, 19 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > index 5e9fd902cd37..92b69a6529fb 100644
> > > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > > @@ -7,6 +7,7 @@
> > >
> > > #include <media/v4l2-ioctl.h>
> > > #include <media/videobuf2-core.h>
> > > +#include <media/videobuf2-dma-contig.h>
> > > #include <media/videobuf2-vmalloc.h>
> > >
> > > #include "vimc-common.h"
> > > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > > /* Initialize the vb2 queue */
> > > q = &vcap->queue;
> > > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > > + q->io_modes = VB2_MMAP | VB2_DMABUF;
> >
> > maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
>
> Why so ? vb2-vmalloc can import dma-bufs.
Indeed, should be safe with both allocator, the CMA one will validate that the
pages are contiguous and fail synchronously as expected for CMA. The known
issues are mostly for reading importers (consumers), for writing importer
(producers) it is likely still a bit buggy on cache management.
> > > if (ret) {
> > > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > > index a289434e75ba..b77939123501 100644
> > > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > > @@ -35,6 +35,8 @@
> > >
> > > #define VIMC_PIX_FMT_MAX_CODES 8
> > >
> > > +extern unsigned int vimc_allocator;
> > > +
> > > /**
> > > * vimc_colorimetry_clamp - Adjust colorimetry parameters
> > > *
> > > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > > index 4b0ae6f51d76..7badcecb7aed 100644
> > > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > > @@ -5,6 +5,7 @@
> > > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> > > */
> > >
> > > +#include <linux/dma-mapping.h>
> > > #include <linux/font.h>
> > > #include <linux/init.h>
> > > #include <linux/module.h>
> > > @@ -15,6 +16,12 @@
> > >
> > > #include "vimc-common.h"
> > >
> > > +unsigned int vimc_allocator;
> > > +module_param_named(allocator, vimc_allocator, uint, 0444);
> > > +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> > > + "\t\t 0 == vmalloc\n"
> > > + "\t\t 1 == dma-contig");
> > > +
> >
> > There is a section 'Module options' in vimc.rst. So a doc should be added there.
>
> OK, I'll update that.
>
> > > #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> > >
> > > #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > > @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
> > >
> > > tpg_set_font(font->data);
> > >
> > > + if (vimc_allocator == 1)
> > > + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > > +
> > > vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> > > if (!vimc)
> > > return -ENOMEM;
> > >
>
^ permalink raw reply
* Re: [GIT PULL v2 for 5.15] Camera sensor, async and documentation patches
From: Mauro Carvalho Chehab @ 2021-07-30 14:46 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
In-Reply-To: <20210727091551.GF3@valkosipuli.retiisi.eu>
Em Tue, 27 Jul 2021 12:15:51 +0300
Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> Hi Mauro,
>
> Here's a large set of patches for 5.15.
>
> Notable changes include:
>
> - Drivers for imx335, imx412 and ov9282 sensors
> - Fixes for digital gain configuration in ov2740 and ov9734 drivers
> - Fix ov8856 driver for ACPI
> - Shorten V4L2 async notifier functions for better fitting under 80 chars
> - Documentation improvements (camera sensors, CSI-2)
> - V4L2 LED flash fixes
> - Manual CSI-2 LP-11/LP-111 mode support with runtime PM, CCS driver
> support
> - Correct mbus code for YUV output in ov5640 driver with CSI-2
> - Switch to DEVICE_ATTR_RO and DEVICE_ATTR_RW in MC and a few drivers
> - Omap3isp error path bugfix
>
> since v1:
>
> - Rebased on media-stage tree
> - Include Niklas's rcar-vin patches
> - Added imx258 fixes by Umang Jain and Laurent Pinchart
>
> Please pull.
Partially applied.
The RCar patches were adding new warnings. So, I ended not applying
them:
0008-0043-media-dt-bindings-media-renesas-csi2-Add-r8a779a0-su.patch
0009-0043-rcar-csi2-Add-r8a779a0-support.patch
0010-0043-rcar-vin-Refactor-controls-creation-for-video-device.patch
0011-0043-rcar-vin-Fix-error-paths-for-rvin_mc_init.patch
0012-0043-rcar-vin-Improve-async-notifier-cleanup-paths.patch
0013-0043-rcar-vin-Improve-reuse-of-parallel-notifier.patch
0014-0043-rcar-vin-Rename-array-storing-subdevice-information.patch
0015-0043-rcar-vin-Move-group-async-notifier.patch
0016-0043-rcar-vin-Extend-group-notifier-DT-parser-to-work-wit.patch
0017-0043-rcar-vin-Create-a-callback-to-setup-media-links.patch
0018-0043-rcar-vin-Specify-media-device-ops-at-group-creation-.patch
0019-0043-rcar-vin-Move-and-rename-CSI-2-link-notifications.patch
0020-0043-rcar-vin-Add-r8a779a0-support.patch
Maybe due to that, those patches also didn't apply:
0029-0043-v4l-async-Rename-async-nf-functions-clean-up-long-li.patch
0030-0043-media-rcar-vin-Remove-explicit-device-availability-c.patch
0031-0043-media-v4l2-fwnode-Simplify-v4l2_async_nf_parse_fwnod.patch
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH] media: vivid: drop CONFIG_FB dependency
From: Guillaume Tucker @ 2021-07-30 14:26 UTC (permalink / raw)
To: kernel test robot, Hans Verkuil, Mauro Carvalho Chehab
Cc: kbuild-all, linux-media, linux-kernel, kernel
In-Reply-To: <202107302145.AQPuE7DD-lkp@intel.com>
On 30/07/2021 14:32, kernel test robot wrote:
> Hi Guillaume,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on v5.14-rc3 next-20210729]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Guillaume-Tucker/media-vivid-drop-CONFIG_FB-dependency/20210729-191815
> base: git://linuxtv.org/media_tree.git master
> config: m68k-allmodconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 10.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/0day-ci/linux/commit/51defc67cada10450046e4d4e7eda1a2573371cc
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Guillaume-Tucker/media-vivid-drop-CONFIG_FB-dependency/20210729-191815
> git checkout 51defc67cada10450046e4d4e7eda1a2573371cc
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>, old ones prefixed by <<):
>
>>> ERROR: modpost: "vivid_clear_fb" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
>>> ERROR: modpost: "vivid_fb_release_buffers" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
>>> ERROR: modpost: "vivid_fb_init" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
Pretty sure this is due to the conditional in the Makefile I
mentioned in an email yesterday, where it should have been
ifneq ($(CONFIG_FB),) for when CONFIG_FB=m.
Let me know if I should send a v2 now with this fix, I was
waiting for Hans' feedback first.
Thanks,
Guillaume
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Laurent Pinchart @ 2021-07-30 14:15 UTC (permalink / raw)
To: Dafna Hirschfeld; +Cc: linux-media, linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <69e4b3fe-5e31-840a-9ea6-a3050f9a6a20@collabora.com>
Hi Dafna,
On Fri, Jul 30, 2021 at 04:08:11PM +0200, Dafna Hirschfeld wrote:
> On 30.07.21 15:11, Laurent Pinchart wrote:
> > On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
> >> On 30.07.21 02:19, Laurent Pinchart wrote:
> >>> The vimc driver is used for testing purpose, and some test use cases
> >>> involve sharing buffers with a consumer device. Consumers often require
> >>> DMA contiguous memory, which vimc doesn't currently support. This leads
> >>> in the best case to usage of bounce buffers, which is very slow, and in
> >>> the worst case in a complete failure.
> >>>
> >>> Add support for the dma-contig allocator in vimc to support those use
> >>> cases properly. The allocator is selected through a new "allocator"
> >>> module parameter, which defaults to vmalloc.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >>> ---
> >>> drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> >>> drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> >>> drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> >>> 3 files changed, 19 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> >>> index 5e9fd902cd37..92b69a6529fb 100644
> >>> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> >>> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> >>> @@ -7,6 +7,7 @@
> >>>
> >>> #include <media/v4l2-ioctl.h>
> >>> #include <media/videobuf2-core.h>
> >>> +#include <media/videobuf2-dma-contig.h>
> >>> #include <media/videobuf2-vmalloc.h>
> >>>
> >>> #include "vimc-common.h"
> >>> @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> >>> /* Initialize the vb2 queue */
> >>> q = &vcap->queue;
> >>> q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> >>> + q->io_modes = VB2_MMAP | VB2_DMABUF;
> >>
> >> maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
> >
> > Why so ? vb2-vmalloc can import dma-bufs.
>
> oh, I meant that exporting should not be supported.
> I see that vimc set ".vidioc_expbuf = vb2_ioctl_expbuf", maybe remove that if allocator is vmalloc?
If the importer support non-contiguous buffers, vb2-vmalloc can be used
as an exporter. I've successfully used this to test sharing buffers
between vimc in vmalloc mode and the R-Car H3 display driver with an
IOMMU.
> >>> + if (vimc_allocator != 1)
> >>
> >> maybe define a macro instead of `1` ?
> >
> > Good idea.
> >
> >>> + q->io_modes |= VB2_USERPTR;
> >>> q->drv_priv = vcap;
> >>> q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> >>> q->ops = &vimc_cap_qops;
> >>> - q->mem_ops = &vb2_vmalloc_memops;
> >>> + q->mem_ops = vimc_allocator == 1
> >>> + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
> >>> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> >>> q->min_buffers_needed = 2;
> >>> q->lock = &vcap->lock;
> >>> + q->dev = v4l2_dev->dev;
> >>>
> >>> ret = vb2_queue_init(q);
> >>> if (ret) {
> >>> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> >>> index a289434e75ba..b77939123501 100644
> >>> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> >>> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> >>> @@ -35,6 +35,8 @@
> >>>
> >>> #define VIMC_PIX_FMT_MAX_CODES 8
> >>>
> >>> +extern unsigned int vimc_allocator;
> >>> +
> >>> /**
> >>> * vimc_colorimetry_clamp - Adjust colorimetry parameters
> >>> *
> >>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> >>> index 4b0ae6f51d76..7badcecb7aed 100644
> >>> --- a/drivers/media/test-drivers/vimc/vimc-core.c
> >>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> >>> @@ -5,6 +5,7 @@
> >>> * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> >>> */
> >>>
> >>> +#include <linux/dma-mapping.h>
> >>> #include <linux/font.h>
> >>> #include <linux/init.h>
> >>> #include <linux/module.h>
> >>> @@ -15,6 +16,12 @@
> >>>
> >>> #include "vimc-common.h"
> >>>
> >>> +unsigned int vimc_allocator;
> >>> +module_param_named(allocator, vimc_allocator, uint, 0444);
> >>> +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> >>> + "\t\t 0 == vmalloc\n"
> >>> + "\t\t 1 == dma-contig");
> >>> +
> >>
> >> There is a section 'Module options' in vimc.rst. So a doc should be added there.
> >
> > OK, I'll update that.
> >
> >>> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> >>>
> >>> #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> >>> @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
> >>>
> >>> tpg_set_font(font->data);
> >>>
> >>> + if (vimc_allocator == 1)
> >>> + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> >>> +
> >>> vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> >>> if (!vimc)
> >>> return -ENOMEM;
> >>>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Dafna Hirschfeld @ 2021-07-30 14:08 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <YQP6aILfBDLhSFUt@pendragon.ideasonboard.com>
On 30.07.21 15:11, Laurent Pinchart wrote:
> Hi Dafna,
>
> On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
>> On 30.07.21 02:19, Laurent Pinchart wrote:
>>> The vimc driver is used for testing purpose, and some test use cases
>>> involve sharing buffers with a consumer device. Consumers often require
>>> DMA contiguous memory, which vimc doesn't currently support. This leads
>>> in the best case to usage of bounce buffers, which is very slow, and in
>>> the worst case in a complete failure.
>>>
>>> Add support for the dma-contig allocator in vimc to support those use
>>> cases properly. The allocator is selected through a new "allocator"
>>> module parameter, which defaults to vmalloc.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>> ---
>>> drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
>>> drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
>>> drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
>>> 3 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
>>> index 5e9fd902cd37..92b69a6529fb 100644
>>> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
>>> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
>>> @@ -7,6 +7,7 @@
>>>
>>> #include <media/v4l2-ioctl.h>
>>> #include <media/videobuf2-core.h>
>>> +#include <media/videobuf2-dma-contig.h>
>>> #include <media/videobuf2-vmalloc.h>
>>>
>>> #include "vimc-common.h"
>>> @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>>> /* Initialize the vb2 queue */
>>> q = &vcap->queue;
>>> q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
>>> + q->io_modes = VB2_MMAP | VB2_DMABUF;
>>
>> maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
>
> Why so ? vb2-vmalloc can import dma-bufs.
oh, I meant that exporting should not be supported.
I see that vimc set ".vidioc_expbuf = vb2_ioctl_expbuf", maybe remove that if allocator is vmalloc?
Thanks,
Dafna
>
>>> + if (vimc_allocator != 1)
>>
>> maybe define a macro instead of `1` ?
>
> Good idea.
>
>>> + q->io_modes |= VB2_USERPTR;
>>> q->drv_priv = vcap;
>>> q->buf_struct_size = sizeof(struct vimc_cap_buffer);
>>> q->ops = &vimc_cap_qops;
>>> - q->mem_ops = &vb2_vmalloc_memops;
>>> + q->mem_ops = vimc_allocator == 1
>>> + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
>>> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>> q->min_buffers_needed = 2;
>>> q->lock = &vcap->lock;
>>> + q->dev = v4l2_dev->dev;
>>>
>>> ret = vb2_queue_init(q);
>>> if (ret) {
>>> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
>>> index a289434e75ba..b77939123501 100644
>>> --- a/drivers/media/test-drivers/vimc/vimc-common.h
>>> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
>>> @@ -35,6 +35,8 @@
>>>
>>> #define VIMC_PIX_FMT_MAX_CODES 8
>>>
>>> +extern unsigned int vimc_allocator;
>>> +
>>> /**
>>> * vimc_colorimetry_clamp - Adjust colorimetry parameters
>>> *
>>> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
>>> index 4b0ae6f51d76..7badcecb7aed 100644
>>> --- a/drivers/media/test-drivers/vimc/vimc-core.c
>>> +++ b/drivers/media/test-drivers/vimc/vimc-core.c
>>> @@ -5,6 +5,7 @@
>>> * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>>> */
>>>
>>> +#include <linux/dma-mapping.h>
>>> #include <linux/font.h>
>>> #include <linux/init.h>
>>> #include <linux/module.h>
>>> @@ -15,6 +16,12 @@
>>>
>>> #include "vimc-common.h"
>>>
>>> +unsigned int vimc_allocator;
>>> +module_param_named(allocator, vimc_allocator, uint, 0444);
>>> +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
>>> + "\t\t 0 == vmalloc\n"
>>> + "\t\t 1 == dma-contig");
>>> +
>>
>> There is a section 'Module options' in vimc.rst. So a doc should be added there.
>
> OK, I'll update that.
>
>>> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
>>>
>>> #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
>>> @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
>>>
>>> tpg_set_font(font->data);
>>>
>>> + if (vimc_allocator == 1)
>>> + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>>> +
>>> vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
>>> if (!vimc)
>>> return -ENOMEM;
>>>
>
^ permalink raw reply
* Re: [PATCH v4] media: vimc: Enable set resolution at the scaler src pad
From: Laurent Pinchart @ 2021-07-30 13:55 UTC (permalink / raw)
To: Helen Koike
Cc: Pedro Terra Delboni, Hans Verkuil, Dafna Hirschfeld,
Mauro Carvalho Chehab, Shuah Khan, linux-media, linux-kernel,
lkcamp, Gabriela Bittencourt, Gabriel Francisco Mandaji
In-Reply-To: <99372b94-241c-f0d2-b1db-c4fa3d423018@collabora.com>
Hello,
Reviving an old thread.
On Tue, Jan 21, 2020 at 10:42:10AM -0300, Helen Koike wrote:
> On 1/17/20 7:56 PM, Pedro Terra Delboni wrote:
> > Sorry to keep bumping this thread, but while doing some testing I got
> > confused with the following:
> > The documentation mentions that the scaler should be reset whenever
> > the sink format is set.
> > Does this mean that I should reset it independently if the sink set
> > changes the dimensions?
> > For example: if I change the pixels from RGB to another format, should
> > that also reset the source dimensions?
>
> Confusing indeed.
>
> My understanding is:
> If you make any changes in the sink pad that would affect the scaling
> ratio, then you should adjust the source pad to make a scaling ration
> 1:1.
> So, if you only change the pixel format, it shouldn't reset the scaling
> ratio.
It's not clearly described in the documentation, but I would have done
the opposite, resetting the scaler to 1:1 every time S_FMT is called on
the sink pad, regardless of what is being changed.
> > Another thing that got me confused was:
> > At by the functions names, setting the crop dimensions is not
> > considered changing the pad format, it's considered setting a selection.
> > Should I also update the set_selection function to propagate the
> > changes to the source pad?
>
> Following my understanding above, yes.
I disagree here too :-) The crop rectangle on the source pad is used to
control digital zoom, during streaming. This requires the output size to
stay the same when the crop rectangle size changes.
Pedro, would you consider submitting a v5 ? Or should I take over ?
> > Otherwise, by changing the crop (without changing the sink format)
> > will cause the scaling to behave in the same way it would if we didn't
> > propagate the sink properties to the source.
> >
> > So:
> > Should I check if the set_fmt is actually changing the sink dimensions
> > in order to propagate them to the source?
>
> Following my understanding above, yes.
>
> > Should I also propagate the dimensions when setting the sink crop?
>
> Following my understanding above, yes.
>
> I wonder now how other drivers are doing this.
>
> At least, I'm sure the driver rkisp1 is not doing the right thing
> and needs to be corrected.
>
> > Sorry for the long email!
> > Thanks for the attention that you've all been giving us so far!
> >
> > On Wed, Jan 15, 2020 at 11:51 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 1/10/20 6:26 PM, Helen Koike wrote:
> >>>
> >>>
> >>> On 1/10/20 3:21 PM, Pedro Terra Delboni wrote:
> >>>> Hello!
> >>>>
> >>>> On Wed, Jan 1, 2020 at 7:10 AM Dafna Hirschfeld
> >>>> <dafna.hirschfeld@collabora.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>>
> >>>>> On 30.12.19 14:59, Helen Koike wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Thanks for the patch, just minor comments below.
> >>>>>>
> >>>>>> On 12/29/19 5:42 PM, Pedro Terra wrote:
> >>>>>>> Modify the scaler subdevice to accept setting the resolution of the source
> >>>>>>> pad (previously the source resolution would always be 3 times the sink for
> >>>>>>> both dimensions). Now any resolution can be set at src (even smaller ones)
> >>>>>>> and the sink video will be scaled to match it.
> >>>>>>>
> >>>>>>> Test example: With the vimc module up (using the default vimc topology)
> >>>>>>> media-ctl -d /dev/media0 -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[fmt:RGB888_1X24/640x480]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":0[crop:(100,50)/400x150]'
> >>>>>>> media-ctl -d /dev/media0 -V '"Scaler":1[fmt:RGB888_1X24/300x700]'
> >>>>>>> v4l2-ctl -d /dev/video2 -v width=300,height=700
> >>>>>>> v4l2-ctl -d /dev/video0 -v pixelformat=BA81
> >>>>>>> v4l2-ctl --stream-mmap --stream-count=10 -d /dev/video2 \
> >>>>>>> --stream-to=test.raw
> >>>>>>> ffplay -loglevel warning -v info -f rawvideo -pixel_format rgb24 \
> >>>>>>> -video_size "300x700" test.raw
> >>>>>>>
> >>>>>>> Co-developed-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> >>>>>>> Signed-off-by: Gabriela Bittencourt <gabrielabittencourt00@gmail.com>
> >>>>>>> Co-developed-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> >>>>>>> Signed-off-by: Gabriel Francisco Mandaji <gfmandaji@gmail.com>
> >>>>>>> Signed-off-by: Pedro "pirate" Terra <pirate@terraco.de>
> >>>>>>>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> Changes in V4:
> >>>>>>> * Rebased with media/master
> >>>>>>> * Scaling is now compatible with crop
> >>>>>>> * Updated test example at the commit message
> >>>>>>> * Add vimc prefix to the pad enumeration
> >>>>>>>
> >>>>>>> Changes in V3:
> >>>>>>> * Corrections suggested by Hans:
> >>>>>>> - Default scaling factor is now 1 (we removed the define and
> >>>>>>> set the source format equals the sink).
> >>>>>>> - Removed SCA_COUNT (enum that represents the number of pads)
> >>>>>>> as there always 2
> >>>>>>> - Swapped the per byte pixel copy to memcpy.
> >>>>>>> * Corrections suggested by Dafna:
> >>>>>>> - Removed from the documentation the old scaler parameter which
> >>>>>>> isn't necessary anymore.
> >>>>>>> * Added a thank you note at the end of the email
> >>>>>>>
> >>>>>>> Changes in V2:
> >>>>>>> * Patch was not sent to media list mail for some reason (even though it
> >>>>>>> was on the Cc list), trying again.
> >>>>>>> * Updating documentation.
> >>>>>>>
> >>>>>>> Hello!
> >>>>>>> This code is the result of friends getting together with too much
> >>>>>>> coffee, sugar and beer trying to get started with some kernel coding.
> >>>>>>> Please, don't go easy on us! s2
> >>>>>>>
> >>>>>>> Running
> >>>>>>> /usr/local/bin/v4l2-compliance -m /dev/media0
> >>>>>>> Gave the following result:
> >>>>>>> v4l2-compliance SHA: b393a5408383b7341883857dfda78537f2f85ef6, 64 bits
> >>>>>>> Grand Total for vimc device /dev/media0: 451, Succeeded: 451, Failed: 0, Warnings: 0
> >>>>>>> ---
> >>>>>>> Documentation/media/v4l-drivers/vimc.rst | 21 +-
> >>>>>>> drivers/media/platform/vimc/vimc-scaler.c | 248 +++++++---------------
> >>>>>>> 2 files changed, 87 insertions(+), 182 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> index 8f5d7f8d83bb..af04ebbd4fa1 100644
> >>>>>>> --- a/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> +++ b/Documentation/media/v4l-drivers/vimc.rst
> >>>>>>> @@ -61,9 +61,11 @@ vimc-debayer:
> >>>>>>> * 1 Pad source
> >>>>>>>
> >>>>>>> vimc-scaler:
> >>>>>>> - Scale up the image by a factor of 3. E.g.: a 640x480 image becomes a
> >>>>>>> - 1920x1440 image. (this value can be configured, see at
> >>>>>>> - `Module options`_).
> >>>>>>> + Re-size the image to meet the source pad resolution. E.g.: if the sync pad
> >>>>>>> +is configured to 360x480 and the source to 1280x720, the image will be stretched
> >>>>>>> +to fit the source resolution. Works for any resolution within the vimc
> >>>>>>> +limitations (even shrinking the image if necessary).
> >>>>>>> +
> >>>>>>> Exposes:
> >>>>>>>
> >>>>>>> * 1 Pad sink
> >>>>>>> @@ -76,19 +78,6 @@ vimc-capture:
> >>>>>>> * 1 Pad sink
> >>>>>>> * 1 Pad source
> >>>>>>>
> >>>>>>> -
> >>>>>>> -Module options
> >>>>>>> ---------------
> >>>>>>> -
> >>>>>>> -Vimc has a module parameter to configure the driver.
> >>>>>>> -
> >>>>>>> -* ``sca_mult=<unsigned int>``
> >>>>>>> -
> >>>>>>> - Image size multiplier factor to be used to multiply both width and
> >>>>>>> - height, so the image size will be ``sca_mult^2`` bigger than the
> >>>>>>> - original one. Currently, only supports scaling up (the default value
> >>>>>>> - is 3).
> >>>>>>> -
> >>>>>>> Source code documentation
> >>>>>>> -------------------------
> >>>>>>>
> >>>>>>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> index e2e551bc3ded..785009b7ac9e 100644
> >>>>>>> --- a/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> >>>>>>> @@ -6,6 +6,7 @@
> >>>>>>> */
> >>>>>>>
> >>>>>>> #include <linux/moduleparam.h>
> >>>>>>> +#include <linux/string.h>
> >>>>>>> #include <linux/vmalloc.h>
> >>>>>>> #include <linux/v4l2-mediabus.h>
> >>>>>>> #include <media/v4l2-rect.h>
> >>>>>>> @@ -13,11 +14,11 @@
> >>>>>>>
> >>>>>>> #include "vimc-common.h"
> >>>>>>>
> >>>>>>> -static unsigned int sca_mult = 3;
> >>>>>>> -module_param(sca_mult, uint, 0000);
> >>>>>>> -MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>>>> -
> >>>>>>> -#define MAX_ZOOM 8
> >>>>>>> +/* Pad identifier */
> >>>>>>> +enum vimc_sca_pad {
> >>>>>>> + VIMC_SCA_SINK = 0,
> >>>>>>> + VIMC_SCA_SRC = 1,
> >>>>>>> +};
> >>>>>>>
> >>>>>>> #define VIMC_SCA_FMT_WIDTH_DEFAULT 640
> >>>>>>> #define VIMC_SCA_FMT_HEIGHT_DEFAULT 480
> >>>>>>> @@ -25,14 +26,11 @@ MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> >>>>>>> struct vimc_sca_device {
> >>>>>>> struct vimc_ent_device ved;
> >>>>>>> struct v4l2_subdev sd;
> >>>>>>> - /* NOTE: the source fmt is the same as the sink
> >>>>>>> - * with the width and hight multiplied by mult
> >>>>>>> - */
> >>>>>>> - struct v4l2_mbus_framefmt sink_fmt;
> >>>>>>> struct v4l2_rect crop_rect;
> >>>>>>> + /* Frame format for both sink and src pad */
> >>>>>>> + struct v4l2_mbus_framefmt fmt[2];
> >>>>>>> /* Values calculated when the stream starts */
> >>>>>>> u8 *src_frame;
> >>>>>>> - unsigned int src_line_size;
> >>>>>>> unsigned int bpp;
> >>>>>>> struct media_pad pads[2];
> >>>>>>> };
> >>>>>>> @@ -90,17 +88,15 @@ static int vimc_sca_init_cfg(struct v4l2_subdev *sd,
> >>>>>>> struct v4l2_rect *r;
> >>>>>>> unsigned int i;
> >>>>>>>
> >>>>>>> - mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> >>>>>>> + mf = v4l2_subdev_get_try_format(sd, cfg, VIMC_SCA_SINK);
> >>>>>>> *mf = sink_fmt_default;
> >>>>>>>
> >>>>>>> - r = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >>>>>>> + r = v4l2_subdev_get_try_crop(sd, cfg, VIMC_SCA_SINK);
> >>>>>>> *r = crop_rect_default;
> >>>>>>>
> >>>>>>> for (i = 1; i < sd->entity.num_pads; i++) {
> >>>>>>> mf = v4l2_subdev_get_try_format(sd, cfg, i);
> >>>>>>> *mf = sink_fmt_default;
> >>>>>>> - mf->width = mf->width * sca_mult;
> >>>>>>> - mf->height = mf->height * sca_mult;
> >>>>>>> }
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> @@ -137,14 +133,8 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
> >>>>>>>
> >>>>>>> fse->min_width = VIMC_FRAME_MIN_WIDTH;
> >>>>>>> fse->min_height = VIMC_FRAME_MIN_HEIGHT;
> >>>>>>> -
> >>>>>>> - if (VIMC_IS_SINK(fse->pad)) {
> >>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>>>> - } else {
> >>>>>>> - fse->max_width = VIMC_FRAME_MAX_WIDTH * MAX_ZOOM;
> >>>>>>> - fse->max_height = VIMC_FRAME_MAX_HEIGHT * MAX_ZOOM;
> >>>>>>> - }
> >>>>>>> + fse->max_width = VIMC_FRAME_MAX_WIDTH;
> >>>>>>> + fse->max_height = VIMC_FRAME_MAX_HEIGHT;
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>> @@ -154,95 +144,73 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
> >>>>>>> struct v4l2_subdev_format *format)
> >>>>>>> {
> >>>>>>> struct vimc_sca_device *vsca = v4l2_get_subdevdata(sd);
> >>>>>>> - struct v4l2_rect *crop_rect;
> >>>>>>>
> >>>>>>> - /* Get the current sink format */
> >>>>>>> - if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> >>>>>>> - format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
> >>>>>>> - crop_rect = v4l2_subdev_get_try_crop(sd, cfg, 0);
> >>>>>>> - } else {
> >>>>>>> - format->format = vsca->sink_fmt;
> >>>>>>> - crop_rect = &vsca->crop_rect;
> >>>>>>> - }
> >>>>>>> -
> >>>>>>> - /* Scale the frame size for the source pad */
> >>>>>>> - if (VIMC_IS_SRC(format->pad)) {
> >>>>>>> - format->format.width = crop_rect->width * sca_mult;
> >>>>>>> - format->format.height = crop_rect->height * sca_mult;
> >>>>>>> - }
> >>>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> >>>>>>> + format->format = *v4l2_subdev_get_try_format(sd, cfg,
> >>>>>>> + format->pad);
> >>>>>>> + else
> >>>>>>> + format->format = vsca->fmt[format->pad];
> >>>>>>>
> >>>>>>> return 0;
> >>>>>>> }
> >>>>>>>
> >>>>>>> -static void vimc_sca_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
> >>>>>>> +static void vimc_sca_adjust_fmt(struct v4l2_mbus_framefmt *fmt[], __u32 pad)
> >>>>>>> {
> >>>>>>> - const struct vimc_pix_map *vpix;
> >>>>>>> + if (pad == VIMC_SCA_SINK) {
> >>>>>>> + const struct vimc_pix_map *vpix;
> >>>>>>>
> >>>>>>> - /* Only accept code in the pix map table in non bayer format */
> >>>>>>> - vpix = vimc_pix_map_by_code(fmt->code);
> >>>>>>> - if (!vpix || vpix->bayer)
> >>>>>>> - fmt->code = sink_fmt_default.code;
> >>>>>>> + /* Only accept code in the pix map table in non bayer format */
> >>>>>>> + vpix = vimc_pix_map_by_code(fmt[VIMC_SCA_SINK]->code);
> >>>>>>> + if (!vpix || vpix->bayer)
> >>>>>>> + fmt[VIMC_SCA_SINK]->code = sink_fmt_default.code;
> >>>>>>> + if (fmt[VIMC_SCA_SINK]->field == V4L2_FIELD_ANY)
> >>>>>>> + fmt[VIMC_SCA_SINK]->field = sink_fmt_default.field;
> >>>>>>>
> >>>>>>> - fmt->width = clamp_t(u32, fmt->width, VIMC_FRAME_MIN_WIDTH,
> >>>>>>> + vimc_colorimetry_clamp(fmt[VIMC_SCA_SINK]);
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + fmt[pad]->width = clamp_t(u32, fmt[pad]->width, VIMC_FRAME_MIN_WIDTH,
> >>>>>>> VIMC_FRAME_MAX_WIDTH) & ~1;
> >>>>>>
> >>>>>> Could you fix the alignment here?
> >>>>>> For some reason checkpatch doesn't catch this :(
> >>>>>>
> >>>>>>> - fmt->height = clamp_t(u32, fmt->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>>>> + fmt[pad]->height = clamp_t(u32, fmt[pad]->height, VIMC_FRAME_MIN_HEIGHT,
> >>>>>>> VIMC_FRAME_MAX_HEIGHT) & ~1;
> >>>>>>
> >>>>>> Also here.
> >>>>>>
> >>>>>>>
> >>>>>>> - if (fmt->field == V4L2_FIELD_ANY)
> >>>>>>> - fmt->field = sink_fmt_default.field;
> >>>>>>> -
> >>>>>>> - vimc_colorimetry_clamp(fmt);
> >>>>>>> + /* Assure src pad attributes besides dimensions are the same as sink */
> >>>>>>> + fmt[VIMC_SCA_SRC]->code = fmt[VIMC_SCA_SINK]->code;
> >>>>>>> + fmt[VIMC_SCA_SRC]->field = fmt[VIMC_SCA_SINK]->field;
> >>>>>>> + fmt[VIMC_SCA_SRC]->colorspace = fmt[VIMC_SCA_SINK]->colorspace;
> >>>>>>
> >>>>>> Ideally we should propagate all the other fields to src. Maybe save width and height to
> >>>>>> a tmp var, assing the whole sink fmt to src, and restore width and height.
> >>>>>>
> >>>>> Acctually according to the subdevices documentation, when changing the
> >>>>> sink format, the width and height of the src format should reset to the
> >>>>> same values:
> >>>>>
> >>>>> ""
> >>>>> - Sub-devices that scale frames using variable scaling factors should
> >>>>> reset the scale factors to default values when sink pads formats are
> >>>>> modified. If the 1:1 scaling ratio is supported, this means that
> >>>>> source pads formats should be reset to the sink pads formats.
> >>>>> ""
> >>>>
> >>>> I have a small question: Should I worry about the crop? I believe that
> >>>> in the current
> >>>> implementation setting the sink does not necessarily reset the crop zone.
> >>>> Should we reset to the sink resolution or to the one determined by the crop?
> >>>> With that said, the way we implemented the scaller, setting a crop
> >>>> does not affect the
> >>>> source resolution (it retains the sink dimensions), should we change this too?
> >>>
> >>> From the docs, it seems that the idea is to keep 1:1 scaling ration, so if you
> >>> have crop in the sink, then the source pad should have dimentions of the crop.
> >>
> >> Correct.
> >>
> >>>
> >>> At least that is my understanding, and the docs should be updated to make it more
> >>> clear.
> >>
> >> Patches are welcome :-)
> >>
> >>>
> >>> I would like to hear other people's opinions on this.
> >>
> >> I agree with you, this is my understanding as well.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] media: vivid: drop CONFIG_FB dependency
From: kernel test robot @ 2021-07-30 13:32 UTC (permalink / raw)
To: Guillaume Tucker, Hans Verkuil, Mauro Carvalho Chehab
Cc: kbuild-all, linux-media, linux-kernel, kernel
In-Reply-To: <bf74a4670438864ca2e6bde47121554490350729.1627557341.git.guillaume.tucker@collabora.com>
[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]
Hi Guillaume,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.14-rc3 next-20210729]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Guillaume-Tucker/media-vivid-drop-CONFIG_FB-dependency/20210729-191815
base: git://linuxtv.org/media_tree.git master
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/51defc67cada10450046e4d4e7eda1a2573371cc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guillaume-Tucker/media-vivid-drop-CONFIG_FB-dependency/20210729-191815
git checkout 51defc67cada10450046e4d4e7eda1a2573371cc
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "vivid_clear_fb" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
>> ERROR: modpost: "vivid_fb_release_buffers" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
>> ERROR: modpost: "vivid_fb_init" [drivers/media/test-drivers/vivid/vivid.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60244 bytes --]
^ permalink raw reply
* [PATCH v2] media: vimc: Add support for contiguous DMA buffers
From: Laurent Pinchart @ 2021-07-30 13:18 UTC (permalink / raw)
To: linux-media; +Cc: linux-renesas-soc, Helen Koike, Shuah Khan, Dafna Hirschfeld
The vimc driver is used for testing purpose, and some test use cases
involve sharing buffers with a consumer device. Consumers often require
DMA contiguous memory, which vimc doesn't currently support. This leads
in the best case to usage of bounce buffers, which is very slow, and in
the worst case in a complete failure.
Add support for the dma-contig allocator in vimc to support those use
cases properly. The allocator is selected through a new "allocator"
module parameter, which defaults to vmalloc.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
Changes since v1:
- Add vimc_allocator_type enum
- Document the new parameter in vimc.rst
---
Documentation/admin-guide/media/vimc.rst | 10 +++++++++-
drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
drivers/media/test-drivers/vimc/vimc-common.h | 7 +++++++
drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
4 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/Documentation/admin-guide/media/vimc.rst b/Documentation/admin-guide/media/vimc.rst
index 211cc8972410..df57f67fef5f 100644
--- a/Documentation/admin-guide/media/vimc.rst
+++ b/Documentation/admin-guide/media/vimc.rst
@@ -80,7 +80,15 @@ vimc-capture:
Module options
--------------
-Vimc has a module parameter to configure the driver.
+Vimc has module parameters to configure the driver.
+
+* ``allocator=<unsigned int>``
+
+ memory allocator selection, default is 0. It specifies the way buffers
+ will be allocated.
+
+ - 0: vmalloc
+ - 1: dma-contig
* ``sca_mult=<unsigned int>``
diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 5e9fd902cd37..d1e2d0739c00 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -7,6 +7,7 @@
#include <media/v4l2-ioctl.h>
#include <media/videobuf2-core.h>
+#include <media/videobuf2-dma-contig.h>
#include <media/videobuf2-vmalloc.h>
#include "vimc-common.h"
@@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
/* Initialize the vb2 queue */
q = &vcap->queue;
q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
- q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
+ q->io_modes = VB2_MMAP | VB2_DMABUF;
+ if (vimc_allocator == VIMC_ALLOCATOR_VMALLOC)
+ q->io_modes |= VB2_USERPTR;
q->drv_priv = vcap;
q->buf_struct_size = sizeof(struct vimc_cap_buffer);
q->ops = &vimc_cap_qops;
- q->mem_ops = &vb2_vmalloc_memops;
+ q->mem_ops = vimc_allocator == VIMC_ALLOCATOR_DMA_CONTIG
+ ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &vcap->lock;
+ q->dev = v4l2_dev->dev;
ret = vb2_queue_init(q);
if (ret) {
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index a289434e75ba..ba1930772589 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -35,6 +35,13 @@
#define VIMC_PIX_FMT_MAX_CODES 8
+extern unsigned int vimc_allocator;
+
+enum vimc_allocator_type {
+ VIMC_ALLOCATOR_VMALLOC = 0,
+ VIMC_ALLOCATOR_DMA_CONTIG = 1,
+};
+
/**
* vimc_colorimetry_clamp - Adjust colorimetry parameters
*
diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
index 4b0ae6f51d76..06edf9d4d92c 100644
--- a/drivers/media/test-drivers/vimc/vimc-core.c
+++ b/drivers/media/test-drivers/vimc/vimc-core.c
@@ -5,6 +5,7 @@
* Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
*/
+#include <linux/dma-mapping.h>
#include <linux/font.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -15,6 +16,12 @@
#include "vimc-common.h"
+unsigned int vimc_allocator;
+module_param_named(allocator, vimc_allocator, uint, 0444);
+MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
+ "\t\t 0 == vmalloc\n"
+ "\t\t 1 == dma-contig");
+
#define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
#define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
@@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
tpg_set_font(font->data);
+ if (vimc_allocator == VIMC_ALLOCATOR_DMA_CONTIG)
+ dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+
vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
if (!vimc)
return -ENOMEM;
--
Regards,
Laurent Pinchart
^ permalink raw reply related
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Laurent Pinchart @ 2021-07-30 13:11 UTC (permalink / raw)
To: Dafna Hirschfeld; +Cc: linux-media, linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <40a1fed8-456e-97c5-9aa7-715a4a4c816b@collabora.com>
Hi Dafna,
On Fri, Jul 30, 2021 at 02:35:20PM +0200, Dafna Hirschfeld wrote:
> On 30.07.21 02:19, Laurent Pinchart wrote:
> > The vimc driver is used for testing purpose, and some test use cases
> > involve sharing buffers with a consumer device. Consumers often require
> > DMA contiguous memory, which vimc doesn't currently support. This leads
> > in the best case to usage of bounce buffers, which is very slow, and in
> > the worst case in a complete failure.
> >
> > Add support for the dma-contig allocator in vimc to support those use
> > cases properly. The allocator is selected through a new "allocator"
> > module parameter, which defaults to vmalloc.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> > drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> > drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> > 3 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> > index 5e9fd902cd37..92b69a6529fb 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> > @@ -7,6 +7,7 @@
> >
> > #include <media/v4l2-ioctl.h>
> > #include <media/videobuf2-core.h>
> > +#include <media/videobuf2-dma-contig.h>
> > #include <media/videobuf2-vmalloc.h>
> >
> > #include "vimc-common.h"
> > @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> > /* Initialize the vb2 queue */
> > q = &vcap->queue;
> > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> > - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> > + q->io_modes = VB2_MMAP | VB2_DMABUF;
>
> maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
Why so ? vb2-vmalloc can import dma-bufs.
> > + if (vimc_allocator != 1)
>
> maybe define a macro instead of `1` ?
Good idea.
> > + q->io_modes |= VB2_USERPTR;
> > q->drv_priv = vcap;
> > q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> > q->ops = &vimc_cap_qops;
> > - q->mem_ops = &vb2_vmalloc_memops;
> > + q->mem_ops = vimc_allocator == 1
> > + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
> > q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > q->min_buffers_needed = 2;
> > q->lock = &vcap->lock;
> > + q->dev = v4l2_dev->dev;
> >
> > ret = vb2_queue_init(q);
> > if (ret) {
> > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> > index a289434e75ba..b77939123501 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-common.h
> > +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> > @@ -35,6 +35,8 @@
> >
> > #define VIMC_PIX_FMT_MAX_CODES 8
> >
> > +extern unsigned int vimc_allocator;
> > +
> > /**
> > * vimc_colorimetry_clamp - Adjust colorimetry parameters
> > *
> > diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> > index 4b0ae6f51d76..7badcecb7aed 100644
> > --- a/drivers/media/test-drivers/vimc/vimc-core.c
> > +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> > @@ -5,6 +5,7 @@
> > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> > */
> >
> > +#include <linux/dma-mapping.h>
> > #include <linux/font.h>
> > #include <linux/init.h>
> > #include <linux/module.h>
> > @@ -15,6 +16,12 @@
> >
> > #include "vimc-common.h"
> >
> > +unsigned int vimc_allocator;
> > +module_param_named(allocator, vimc_allocator, uint, 0444);
> > +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> > + "\t\t 0 == vmalloc\n"
> > + "\t\t 1 == dma-contig");
> > +
>
> There is a section 'Module options' in vimc.rst. So a doc should be added there.
OK, I'll update that.
> > #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> >
> > #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> > @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
> >
> > tpg_set_font(font->data);
> >
> > + if (vimc_allocator == 1)
> > + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > +
> > vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> > if (!vimc)
> > return -ENOMEM;
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] media: vimc: Add support for contiguous DMA buffers
From: Dafna Hirschfeld @ 2021-07-30 12:35 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Helen Koike, Shuah Khan
In-Reply-To: <20210730001939.30769-1-laurent.pinchart+renesas@ideasonboard.com>
Hi
On 30.07.21 02:19, Laurent Pinchart wrote:
> The vimc driver is used for testing purpose, and some test use cases
> involve sharing buffers with a consumer device. Consumers often require
> DMA contiguous memory, which vimc doesn't currently support. This leads
> in the best case to usage of bounce buffers, which is very slow, and in
> the worst case in a complete failure.
>
> Add support for the dma-contig allocator in vimc to support those use
> cases properly. The allocator is selected through a new "allocator"
> module parameter, which defaults to vmalloc.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> drivers/media/test-drivers/vimc/vimc-capture.c | 9 +++++++--
> drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
> drivers/media/test-drivers/vimc/vimc-core.c | 10 ++++++++++
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 5e9fd902cd37..92b69a6529fb 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -7,6 +7,7 @@
>
> #include <media/v4l2-ioctl.h>
> #include <media/videobuf2-core.h>
> +#include <media/videobuf2-dma-contig.h>
> #include <media/videobuf2-vmalloc.h>
>
> #include "vimc-common.h"
> @@ -423,14 +424,18 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> /* Initialize the vb2 queue */
> q = &vcap->queue;
> q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> - q->io_modes = VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> + q->io_modes = VB2_MMAP | VB2_DMABUF;
maybe to be on the safe side VB2_DMABUF should be set only if vimc_allocator==1 ?
> + if (vimc_allocator != 1)
maybe define a macro instead of `1` ?
> + q->io_modes |= VB2_USERPTR;
> q->drv_priv = vcap;
> q->buf_struct_size = sizeof(struct vimc_cap_buffer);
> q->ops = &vimc_cap_qops;
> - q->mem_ops = &vb2_vmalloc_memops;
> + q->mem_ops = vimc_allocator == 1
> + ? &vb2_dma_contig_memops : &vb2_vmalloc_memops;
> q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> q->min_buffers_needed = 2;
> q->lock = &vcap->lock;
> + q->dev = v4l2_dev->dev;
>
> ret = vb2_queue_init(q);
> if (ret) {
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
> index a289434e75ba..b77939123501 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -35,6 +35,8 @@
>
> #define VIMC_PIX_FMT_MAX_CODES 8
>
> +extern unsigned int vimc_allocator;
> +
> /**
> * vimc_colorimetry_clamp - Adjust colorimetry parameters
> *
> diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
> index 4b0ae6f51d76..7badcecb7aed 100644
> --- a/drivers/media/test-drivers/vimc/vimc-core.c
> +++ b/drivers/media/test-drivers/vimc/vimc-core.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
> */
>
> +#include <linux/dma-mapping.h>
> #include <linux/font.h>
> #include <linux/init.h>
> #include <linux/module.h>
> @@ -15,6 +16,12 @@
>
> #include "vimc-common.h"
>
> +unsigned int vimc_allocator;
> +module_param_named(allocator, vimc_allocator, uint, 0444);
> +MODULE_PARM_DESC(allocator, " memory allocator selection, default is 0.\n"
> + "\t\t 0 == vmalloc\n"
> + "\t\t 1 == dma-contig");
> +
There is a section 'Module options' in vimc.rst. So a doc should be added there.
Thanks,
Dafna
> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
>
> #define VIMC_ENT_LINK(src, srcpad, sink, sinkpad, link_flags) { \
> @@ -278,6 +285,9 @@ static int vimc_probe(struct platform_device *pdev)
>
> tpg_set_font(font->data);
>
> + if (vimc_allocator == 1)
> + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +
> vimc = kzalloc(sizeof(*vimc), GFP_KERNEL);
> if (!vimc)
> return -ENOMEM;
>
^ permalink raw reply
* [PATCH v2 2/2] media: staging/intel-ipu3: css: Use the struct_size() helper
From: Gustavo A. R. Silva @ 2021-07-30 12:09 UTC (permalink / raw)
To: linux-kernel
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
linux-staging, linux-hardening, Gustavo A. R. Silva,
Dan Carpenter
In-Reply-To: <cover.1627646101.git.gustavoars@kernel.org>
Make use of the struct_size() helper instead of an open-coded version.
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
- Move the replacement of one-element array with a flexible-array
member to patch 1 of the series and update changelog text,
accordingly.
drivers/staging/media/ipu3/ipu3-css-fw.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 630cb5186b48..b9c850fc9fe4 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -127,9 +127,8 @@ int imgu_css_fw_init(struct imgu_css *css)
if (css->fw->size <= sizeof(struct imgu_fw_header) ||
css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
goto bad_fw;
- if (sizeof(struct imgu_fw_bi_file_h) +
- css->fwp->file_header.binary_nr * sizeof(struct imgu_fw_info) >
- css->fw->size)
+ if (struct_size(css->fwp, binary_header,
+ css->fwp->file_header.binary_nr) > css->fw->size)
goto bad_fw;
dev_info(dev, "loaded firmware version %.64s, %u binaries, %zu bytes\n",
--
2.27.0
^ permalink raw reply related
* Aw: [PATCH v7 00/12] Clean up "mediatek,larb"
From: Frank Wunderlich @ 2021-07-30 12:06 UTC (permalink / raw)
To: Yong Wu
Cc: Matthias Brugger, Joerg Roedel, Rob Herring, Krzysztof Kozlowski,
David Airlie, Mauro Carvalho Chehab, Evan Green, Robin Murphy,
Tomasz Figa, Will Deacon, linux-mediatek, srv_heupstream,
devicetree, linux-kernel, linux-arm-kernel, iommu, yong.wu,
youlin.pei, Nicolas Boichat, Matthias Kaehlcke, anan.sun,
ming-fan.chen, yi.kuo, acourbot, linux-media, dri-devel,
Daniel Vetter, Chun-Kuang Hu, Philipp Zabel, Xia Jiang,
Tiffany Lin, Dafna Hirschfeld, Hsin-Yi Wang, Eizan Miyamoto,
anthony.huang
In-Reply-To: <20210730025238.22456-1-yong.wu@mediatek.com>
Full Series tested on BPI-R2/MT7623
Tested-By: Frank Wunderlich <frank-w@public-files.de>
regards Frank
^ permalink raw reply
* [PATCH v2 1/2] media: staging/intel-ipu3: css: Fix wrong size comparison
From: Gustavo A. R. Silva @ 2021-07-30 12:08 UTC (permalink / raw)
To: linux-kernel
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
linux-staging, linux-hardening, Gustavo A. R. Silva,
Dan Carpenter
In-Reply-To: <cover.1627646101.git.gustavoars@kernel.org>
There is a wrong comparison of the total size of the loaded firmware
css->fw->size with the size of a pointer to struct imgu_fw_header.
Fix this by using the right operand 'struct imgu_fw_header' for
sizeof, instead of 'struct imgu_fw_header *' and turn binary_header
into a flexible-array member. Also, adjust the relational operator
to be '<=' instead of '<', as it seems that the intention of the
comparison is to determine if the loaded firmware contains any
'struct imgu_fw_info' items in the binary_header[] array than merely
the file_header (struct imgu_fw_bi_file_h).
The replacement of the one-element array with a flexible-array member
also help with the ongoing efforts to globally enable -Warray-bounds
and get us closer to being able to tighten the FORTIFY_SOURCE routines
on memcpy().
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Fixes: 09d290f0ba21 ("media: staging/intel-ipu3: css: Add support for firmware management")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
It'd be just great if someone that knows this code better can confirm
these changes are correct. In particular the adjustment of the
relational operator. Thanks!
Changes in v2:
- Use flexible array and adjust relational operator, accordingly.
- Update changelog text.
drivers/staging/media/ipu3/ipu3-css-fw.c | 2 +-
drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.c b/drivers/staging/media/ipu3/ipu3-css-fw.c
index 45aff76198e2..630cb5186b48 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.c
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.c
@@ -124,7 +124,7 @@ int imgu_css_fw_init(struct imgu_css *css)
/* Check and display fw header info */
css->fwp = (struct imgu_fw_header *)css->fw->data;
- if (css->fw->size < sizeof(struct imgu_fw_header *) ||
+ if (css->fw->size <= sizeof(struct imgu_fw_header) ||
css->fwp->file_header.h_size != sizeof(struct imgu_fw_bi_file_h))
goto bad_fw;
if (sizeof(struct imgu_fw_bi_file_h) +
diff --git a/drivers/staging/media/ipu3/ipu3-css-fw.h b/drivers/staging/media/ipu3/ipu3-css-fw.h
index 3c078f15a295..c0bc57fd678a 100644
--- a/drivers/staging/media/ipu3/ipu3-css-fw.h
+++ b/drivers/staging/media/ipu3/ipu3-css-fw.h
@@ -171,7 +171,7 @@ struct imgu_fw_bi_file_h {
struct imgu_fw_header {
struct imgu_fw_bi_file_h file_header;
- struct imgu_fw_info binary_header[1]; /* binary_nr items */
+ struct imgu_fw_info binary_header[]; /* binary_nr items */
};
/******************* Firmware functions *******************/
--
2.27.0
^ permalink raw reply related
* [PATCH v2 0/2] Fix size comparison bug and use flexible array
From: Gustavo A. R. Silva @ 2021-07-30 12:07 UTC (permalink / raw)
To: linux-kernel
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab, Greg Kroah-Hartman, linux-media,
linux-staging, linux-hardening, Gustavo A. R. Silva,
Dan Carpenter
Hi all,
This small series aims to fix a size comparison bug and replace a
one-element array with a flexible-array member.
Thanks
Changes in v2:
- Use flexible array and adjust relational operator in patch 1.
Gustavo A. R. Silva (2):
media: staging/intel-ipu3: css: Fix wrong size comparison
media: staging/intel-ipu3: css: Use the struct_size() helper
drivers/staging/media/ipu3/ipu3-css-fw.c | 7 +++----
drivers/staging/media/ipu3/ipu3-css-fw.h | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
--
2.27.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox