| Message ID | e5384b296a0af099dc502572752df149127b7947.1599167568.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: venus: core: Drop local dma_parms | expand |
Hi Robin, On 03/09/2020 22:14, Robin Murphy wrote: > Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms > for platform devices"), struct platform_device already provides a > dma_parms structure, so we can save allocating another one. > > Also the DMA segment size is simply a size, not a bitmask. Well, I guess that answers my previous question ;-) Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/media/platform/qcom/venus/core.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 203c6538044f..2fa9275d75ff 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device *pdev) > if (ret) > return ret; > > - if (!dev->dma_parms) { > - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > - GFP_KERNEL); > - if (!dev->dma_parms) > - return -ENOMEM; > - } > - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + dma_set_max_seg_size(dev, UINT_MAX); > > INIT_LIST_HEAD(&core->instances); > mutex_init(&core->lock); >
Hi Robin, Thanks for the patch! On 9/4/20 12:14 AM, Robin Murphy wrote: > Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms > for platform devices"), struct platform_device already provides a > dma_parms structure, so we can save allocating another one. > > Also the DMA segment size is simply a size, not a bitmask. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/media/platform/qcom/venus/core.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 203c6538044f..2fa9275d75ff 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device *pdev) > if (ret) > return ret; > > - if (!dev->dma_parms) { > - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > - GFP_KERNEL); > - if (!dev->dma_parms) > - return -ENOMEM; > - } > - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + dma_set_max_seg_size(dev, UINT_MAX); To be correct we should check for EIO error? > > INIT_LIST_HEAD(&core->instances); > mutex_init(&core->lock); >
On 2020-09-04 21:26, Stanimir Varbanov wrote: > Hi Robin, > > Thanks for the patch! > > On 9/4/20 12:14 AM, Robin Murphy wrote: >> Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms >> for platform devices"), struct platform_device already provides a >> dma_parms structure, so we can save allocating another one. >> >> Also the DMA segment size is simply a size, not a bitmask. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/media/platform/qcom/venus/core.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index 203c6538044f..2fa9275d75ff 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - if (!dev->dma_parms) { >> - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), >> - GFP_KERNEL); >> - if (!dev->dma_parms) >> - return -ENOMEM; >> - } >> - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >> + dma_set_max_seg_size(dev, UINT_MAX); > > To be correct we should check for EIO error? Well, half the point of 9495b7e92f71 was to make sure that dma_set_max_seg_size() cannot fail for platform drivers. Thus if we're taking advantage of that change to make the assumption that we never need to allocate dma_parms here, then an error check would be redundant by definition ;) Robin. > >> >> INIT_LIST_HEAD(&core->instances); >> mutex_init(&core->lock); >> >
Hi Robin, On 2020-09-04 02:44, Robin Murphy wrote: > Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms > for platform devices"), struct platform_device already provides a > dma_parms structure, so we can save allocating another one. > > Also the DMA segment size is simply a size, not a bitmask. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/media/platform/qcom/venus/core.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/media/platform/qcom/venus/core.c > b/drivers/media/platform/qcom/venus/core.c > index 203c6538044f..2fa9275d75ff 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device > *pdev) > if (ret) > return ret; > > - if (!dev->dma_parms) { > - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > - GFP_KERNEL); > - if (!dev->dma_parms) > - return -ENOMEM; > - } > - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + dma_set_max_seg_size(dev, UINT_MAX); > > INIT_LIST_HEAD(&core->instances); > mutex_init(&core->lock); This reintroduced dma api debug warning which the original commit was addressing or rather thought it addressed. DMA-API: qcom-venus aa00000.video-codec: mapping sg segment longer than device claims to support [len=4194304] [max=65536] WARNING: CPU: 3 PID: 5365 at kernel/dma/debug.c:1225 debug_dma_map_sg+0x1ac/0x2c8 <...> pstate: 60400009 (nZCv daif +PAN -UAO) pc : debug_dma_map_sg+0x1ac/0x2c8 lr : debug_dma_map_sg+0x1ac/0x2c8 sp : ffffff8016517850 x29: ffffff8016517860 x28: 0000000000010000 x27: 00000000ffffffff x26: ffffff80da45eb00 x25: ffffffd03c465000 x24: ffffffd03b3c1000 x23: ffffff803e262d80 x22: ffffff80d9a0d010 x21: 0000000000000001 x20: 0000000000000001 x19: 0000000000000001 x18: 00000000ffff0a10 x17: ffffffd03b84a000 x16: 0000000000000037 x15: ffffffd03a950610 x14: 0000000000000001 x13: 0000000000000000 x12: 00000000a3b31442 x11: 0000000000000000 x10: dfffffd000000001 x9 : f544368f90c5ee00 x8 : f544368f90c5ee00 x7 : ffffffd03af5d570 x6 : 0000000000000000 x5 : 0000000000000080 x4 : 0000000000000001 x3 : ffffffd03a9174b0 x2 : 0000000000000001 x1 : 0000000000000008 x0 : 000000000000007a Call trace: debug_dma_map_sg+0x1ac/0x2c8 vb2_dma_sg_alloc+0x274/0x2f4 [videobuf2_dma_sg] __vb2_queue_alloc+0x14c/0x3b0 [videobuf2_common] vb2_core_reqbufs+0x234/0x374 [videobuf2_common] vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2] v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem] v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem] v4l_reqbufs+0x4c/0x5c __video_do_ioctl+0x2cc/0x3e0 video_usercopy+0x3b0/0x910 video_ioctl2+0x38/0x48 v4l2_ioctl+0x6c/0x80 do_video_ioctl+0xb54/0x2708 v4l2_compat_ioctl32+0x5c/0xcc __se_compat_sys_ioctl+0x100/0x2064 __arm64_compat_sys_ioctl+0x20/0x2c el0_svc_common+0xa8/0x178 el0_svc_compat_handler+0x2c/0x40 el0_svc_compat+0x8/0x10 Thanks, Sai
On 10/2/20 11:06 AM, Sai Prakash Ranjan wrote: > Hi Robin, > > On 2020-09-04 02:44, Robin Murphy wrote: >> Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms >> for platform devices"), struct platform_device already provides a >> dma_parms structure, so we can save allocating another one. >> >> Also the DMA segment size is simply a size, not a bitmask. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/media/platform/qcom/venus/core.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/drivers/media/platform/qcom/venus/core.c >> b/drivers/media/platform/qcom/venus/core.c >> index 203c6538044f..2fa9275d75ff 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - if (!dev->dma_parms) { >> - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), >> - GFP_KERNEL); >> - if (!dev->dma_parms) >> - return -ENOMEM; >> - } >> - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >> + dma_set_max_seg_size(dev, UINT_MAX); >> >> INIT_LIST_HEAD(&core->instances); >> mutex_init(&core->lock); > > This reintroduced dma api debug warning which the original commit was > addressing or rather thought it addressed. > > DMA-API: qcom-venus aa00000.video-codec: mapping sg segment longer than > device claims to support [len=4194304] [max=65536] > WARNING: CPU: 3 PID: 5365 at kernel/dma/debug.c:1225 > debug_dma_map_sg+0x1ac/0x2c8 > <...> > pstate: 60400009 (nZCv daif +PAN -UAO) > pc : debug_dma_map_sg+0x1ac/0x2c8 > lr : debug_dma_map_sg+0x1ac/0x2c8 > sp : ffffff8016517850 > x29: ffffff8016517860 x28: 0000000000010000 > x27: 00000000ffffffff x26: ffffff80da45eb00 > x25: ffffffd03c465000 x24: ffffffd03b3c1000 > x23: ffffff803e262d80 x22: ffffff80d9a0d010 > x21: 0000000000000001 x20: 0000000000000001 > x19: 0000000000000001 x18: 00000000ffff0a10 > x17: ffffffd03b84a000 x16: 0000000000000037 > x15: ffffffd03a950610 x14: 0000000000000001 > x13: 0000000000000000 x12: 00000000a3b31442 > x11: 0000000000000000 x10: dfffffd000000001 > x9 : f544368f90c5ee00 x8 : f544368f90c5ee00 > x7 : ffffffd03af5d570 x6 : 0000000000000000 > x5 : 0000000000000080 x4 : 0000000000000001 > x3 : ffffffd03a9174b0 x2 : 0000000000000001 > x1 : 0000000000000008 x0 : 000000000000007a > Call trace: > debug_dma_map_sg+0x1ac/0x2c8 > vb2_dma_sg_alloc+0x274/0x2f4 [videobuf2_dma_sg] > __vb2_queue_alloc+0x14c/0x3b0 [videobuf2_common] > vb2_core_reqbufs+0x234/0x374 [videobuf2_common] > vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2] > v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem] > v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem] > v4l_reqbufs+0x4c/0x5c > __video_do_ioctl+0x2cc/0x3e0 > video_usercopy+0x3b0/0x910 > video_ioctl2+0x38/0x48 > v4l2_ioctl+0x6c/0x80 > do_video_ioctl+0xb54/0x2708 > v4l2_compat_ioctl32+0x5c/0xcc > __se_compat_sys_ioctl+0x100/0x2064 > __arm64_compat_sys_ioctl+0x20/0x2c > el0_svc_common+0xa8/0x178 > el0_svc_compat_handler+0x2c/0x40 > el0_svc_compat+0x8/0x10 > > Thanks, > Sai > Do you have the mentioned above commit when you see this warning ?
On 2020-10-02 16:57, Stanimir Varbanov wrote: > On 10/2/20 11:06 AM, Sai Prakash Ranjan wrote: >> Hi Robin, >> >> On 2020-09-04 02:44, Robin Murphy wrote: >>> Since commit 9495b7e92f71 ("driver core: platform: Initialize >>> dma_parms >>> for platform devices"), struct platform_device already provides a >>> dma_parms structure, so we can save allocating another one. >>> >>> Also the DMA segment size is simply a size, not a bitmask. >>> >>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>> --- >>> drivers/media/platform/qcom/venus/core.c | 8 +------- >>> 1 file changed, 1 insertion(+), 7 deletions(-) >>> >>> diff --git a/drivers/media/platform/qcom/venus/core.c >>> b/drivers/media/platform/qcom/venus/core.c >>> index 203c6538044f..2fa9275d75ff 100644 >>> --- a/drivers/media/platform/qcom/venus/core.c >>> +++ b/drivers/media/platform/qcom/venus/core.c >>> @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device >>> *pdev) >>> if (ret) >>> return ret; >>> >>> - if (!dev->dma_parms) { >>> - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), >>> - GFP_KERNEL); >>> - if (!dev->dma_parms) >>> - return -ENOMEM; >>> - } >>> - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >>> + dma_set_max_seg_size(dev, UINT_MAX); >>> >>> INIT_LIST_HEAD(&core->instances); >>> mutex_init(&core->lock); >> >> This reintroduced dma api debug warning which the original commit was >> addressing or rather thought it addressed. >> >> DMA-API: qcom-venus aa00000.video-codec: mapping sg segment longer >> than >> device claims to support [len=4194304] [max=65536] >> WARNING: CPU: 3 PID: 5365 at kernel/dma/debug.c:1225 >> debug_dma_map_sg+0x1ac/0x2c8 >> <...> >> pstate: 60400009 (nZCv daif +PAN -UAO) >> pc : debug_dma_map_sg+0x1ac/0x2c8 >> lr : debug_dma_map_sg+0x1ac/0x2c8 >> sp : ffffff8016517850 >> x29: ffffff8016517860 x28: 0000000000010000 >> x27: 00000000ffffffff x26: ffffff80da45eb00 >> x25: ffffffd03c465000 x24: ffffffd03b3c1000 >> x23: ffffff803e262d80 x22: ffffff80d9a0d010 >> x21: 0000000000000001 x20: 0000000000000001 >> x19: 0000000000000001 x18: 00000000ffff0a10 >> x17: ffffffd03b84a000 x16: 0000000000000037 >> x15: ffffffd03a950610 x14: 0000000000000001 >> x13: 0000000000000000 x12: 00000000a3b31442 >> x11: 0000000000000000 x10: dfffffd000000001 >> x9 : f544368f90c5ee00 x8 : f544368f90c5ee00 >> x7 : ffffffd03af5d570 x6 : 0000000000000000 >> x5 : 0000000000000080 x4 : 0000000000000001 >> x3 : ffffffd03a9174b0 x2 : 0000000000000001 >> x1 : 0000000000000008 x0 : 000000000000007a >> Call trace: >> debug_dma_map_sg+0x1ac/0x2c8 >> vb2_dma_sg_alloc+0x274/0x2f4 [videobuf2_dma_sg] >> __vb2_queue_alloc+0x14c/0x3b0 [videobuf2_common] >> vb2_core_reqbufs+0x234/0x374 [videobuf2_common] >> vb2_reqbufs+0x4c/0x64 [videobuf2_v4l2] >> v4l2_m2m_reqbufs+0x50/0x84 [v4l2_mem2mem] >> v4l2_m2m_ioctl_reqbufs+0x2c/0x38 [v4l2_mem2mem] >> v4l_reqbufs+0x4c/0x5c >> __video_do_ioctl+0x2cc/0x3e0 >> video_usercopy+0x3b0/0x910 >> video_ioctl2+0x38/0x48 >> v4l2_ioctl+0x6c/0x80 >> do_video_ioctl+0xb54/0x2708 >> v4l2_compat_ioctl32+0x5c/0xcc >> __se_compat_sys_ioctl+0x100/0x2064 >> __arm64_compat_sys_ioctl+0x20/0x2c >> el0_svc_common+0xa8/0x178 >> el0_svc_compat_handler+0x2c/0x40 >> el0_svc_compat+0x8/0x10 >> >> Thanks, >> Sai >> > > Do you have the mentioned above commit when you see this warning ? +Stephen reported this, this was recently backported to 5.4 kernel where playing youtube with dma api debug enabled would throw this warning and I am almost 100% certain this is the commit which caused the warning to appear again. Thanks, Sai
Quoting Sai Prakash Ranjan (2020-10-02 05:45:03) > On 2020-10-02 16:57, Stanimir Varbanov wrote: > > On 10/2/20 11:06 AM, Sai Prakash Ranjan wrote: > >> On 2020-09-04 02:44, Robin Murphy wrote: > >>> Since commit 9495b7e92f71 ("driver core: platform: Initialize > >>> dma_parms > >>> for platform devices"), struct platform_device already provides a > >>> dma_parms structure, so we can save allocating another one. > >> > > > > Do you have the mentioned above commit when you see this warning ? > > +Stephen reported this, this was recently backported to 5.4 kernel > where playing youtube with dma api debug enabled would throw this > warning and I am almost 100% certain this is the commit which caused > the warning to appear again. > We don't have commit 9495b7e92f71 though so I guess we need that one if we take this patch.
On 2020-10-06 06:32, Stephen Boyd wrote: > Quoting Sai Prakash Ranjan (2020-10-02 05:45:03) >> On 2020-10-02 16:57, Stanimir Varbanov wrote: >> > On 10/2/20 11:06 AM, Sai Prakash Ranjan wrote: >> >> On 2020-09-04 02:44, Robin Murphy wrote: >> >>> Since commit 9495b7e92f71 ("driver core: platform: Initialize >> >>> dma_parms >> >>> for platform devices"), struct platform_device already provides a >> >>> dma_parms structure, so we can save allocating another one. >> >> >> > >> > Do you have the mentioned above commit when you see this warning ? >> >> +Stephen reported this, this was recently backported to 5.4 kernel >> where playing youtube with dma api debug enabled would throw this >> warning and I am almost 100% certain this is the commit which caused >> the warning to appear again. >> > > We don't have commit 9495b7e92f71 though so I guess we need that one > if we take this patch. Oh so Stan was referring to that commit, oops my bad. I thought he was referring to this patch. So I suppose everything is good if we backport both patches. Thanks, Sai
On 2020-10-06 06:23, Sai Prakash Ranjan wrote: > On 2020-10-06 06:32, Stephen Boyd wrote: >> Quoting Sai Prakash Ranjan (2020-10-02 05:45:03) >>> On 2020-10-02 16:57, Stanimir Varbanov wrote: >>> > On 10/2/20 11:06 AM, Sai Prakash Ranjan wrote: >>> >> On 2020-09-04 02:44, Robin Murphy wrote: >>> >>> Since commit 9495b7e92f71 ("driver core: platform: Initialize >>> >>> dma_parms >>> >>> for platform devices"), struct platform_device already provides a >>> >>> dma_parms structure, so we can save allocating another one. >>> >> >>> > >>> > Do you have the mentioned above commit when you see this warning ? >>> >>> +Stephen reported this, this was recently backported to 5.4 kernel >>> where playing youtube with dma api debug enabled would throw this >>> warning and I am almost 100% certain this is the commit which caused >>> the warning to appear again. >>> >> >> We don't have commit 9495b7e92f71 though so I guess we need that one >> if we take this patch. > > Oh so Stan was referring to that commit, oops my bad. I thought > he was referring to this patch. So I suppose everything is good > if we backport both patches. Right, this patch is just some trivial housekeeping which certainly doesn't deserve backporting in its own right. However if it's because you're doing something like backporting an entire chunk of the media tree as a unit, then yeah, you'll need to track down all the functional dependencies, especially any that *weren't* helpfully called out in commit messages ;) Robin.
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 203c6538044f..2fa9275d75ff 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -226,13 +226,7 @@ static int venus_probe(struct platform_device *pdev) if (ret) return ret; - if (!dev->dma_parms) { - dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), - GFP_KERNEL); - if (!dev->dma_parms) - return -ENOMEM; - } - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); + dma_set_max_seg_size(dev, UINT_MAX); INIT_LIST_HEAD(&core->instances); mutex_init(&core->lock);
Since commit 9495b7e92f71 ("driver core: platform: Initialize dma_parms for platform devices"), struct platform_device already provides a dma_parms structure, so we can save allocating another one. Also the DMA segment size is simply a size, not a bitmask. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/media/platform/qcom/venus/core.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)