| Message ID | e5384b296a0af099dc502572752df149127b7947.1599167568.git.robin.murphy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: venus: core: Drop local dma_parms | expand |
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 ? -- regards, Stan
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 -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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 -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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(-)