| Message ID | 20240617152843.11886-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/fbdev-dma: Only set smem_start is enable per module option | expand |
Hi Thomas, On Mon, 17 Jun 2024, Thomas Zimmermann wrote: > Only export struct fb_info.fix.smem_start if that is required by the > user and the memory does not come from vmalloc(). > > Setting struct fb_info.fix.smem_start breaks systems where DMA > memory is backed by vmalloc address space. An example error is > shown below. > > [ 3.536043] ------------[ cut here ]------------ > [ 3.540716] virt_to_phys used for non-linear address: 000000007fc4f540 (0xffff800086001000) > [ 3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 __virt_to_phys+0x68/0x98 > [ 3.565455] Modules linked in: > [ 3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted 6.6.23-06226-g4986cc3e1b75-dirty #250 > [ 3.577310] Hardware name: NXP i.MX95 19X19 board (DT) > [ 3.582452] Workqueue: events_unbound deferred_probe_work_func > [ 3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 3.595233] pc : __virt_to_phys+0x68/0x98 > [ 3.599246] lr : __virt_to_phys+0x68/0x98 > [ 3.603276] sp : ffff800083603990 > [ 3.677939] Call trace: > [ 3.680393] __virt_to_phys+0x68/0x98 > [ 3.684067] drm_fbdev_dma_helper_fb_probe+0x138/0x238 > [ 3.689214] __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0 > [ 3.695385] drm_fb_helper_initial_config+0x4c/0x68 > [ 3.700264] drm_fbdev_dma_client_hotplug+0x8c/0xe0 > [ 3.705161] drm_client_register+0x60/0xb0 > [ 3.709269] drm_fbdev_dma_setup+0x94/0x148 > > Additionally, DMA memory is assumed to by contiguous in physical > address space, which is not guaranteed by vmalloc(). > > Resolve this by checking the module flag drm_leak_fbdev_smem when > DRM allocated the instance of struct fb_info. Fbdev-dma then only > sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also > guarantee that the framebuffer is not located in vmalloc address > space. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Reported-by: Peng Fan (OSS) <peng.fan@oss.nxp.com> > Closes: https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/ > Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers") Thanks, this fixes the issue I was seeing on R-Car Gen3/Gen4 with rcar-du. No regressions on R-Car Gen2 (rcar-du) and R-Mobile A1 (shmobile) which didn't shown the warning in the first place. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Am 17.06.24 um 19:42 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Mon, 17 Jun 2024, Thomas Zimmermann wrote: >> Only export struct fb_info.fix.smem_start if that is required by the >> user and the memory does not come from vmalloc(). >> >> Setting struct fb_info.fix.smem_start breaks systems where DMA >> memory is backed by vmalloc address space. An example error is >> shown below. >> >> [ 3.536043] ------------[ cut here ]------------ >> [ 3.540716] virt_to_phys used for non-linear address: >> 000000007fc4f540 (0xffff800086001000) >> [ 3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 >> __virt_to_phys+0x68/0x98 >> [ 3.565455] Modules linked in: >> [ 3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted >> 6.6.23-06226-g4986cc3e1b75-dirty #250 >> [ 3.577310] Hardware name: NXP i.MX95 19X19 board (DT) >> [ 3.582452] Workqueue: events_unbound deferred_probe_work_func >> [ 3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS >> BTYPE=--) >> [ 3.595233] pc : __virt_to_phys+0x68/0x98 >> [ 3.599246] lr : __virt_to_phys+0x68/0x98 >> [ 3.603276] sp : ffff800083603990 >> [ 3.677939] Call trace: >> [ 3.680393] __virt_to_phys+0x68/0x98 >> [ 3.684067] drm_fbdev_dma_helper_fb_probe+0x138/0x238 >> [ 3.689214] __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0 >> [ 3.695385] drm_fb_helper_initial_config+0x4c/0x68 >> [ 3.700264] drm_fbdev_dma_client_hotplug+0x8c/0xe0 >> [ 3.705161] drm_client_register+0x60/0xb0 >> [ 3.709269] drm_fbdev_dma_setup+0x94/0x148 >> >> Additionally, DMA memory is assumed to by contiguous in physical >> address space, which is not guaranteed by vmalloc(). >> >> Resolve this by checking the module flag drm_leak_fbdev_smem when >> DRM allocated the instance of struct fb_info. Fbdev-dma then only >> sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also >> guarantee that the framebuffer is not located in vmalloc address >> space. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reported-by: Peng Fan (OSS) <peng.fan@oss.nxp.com> >> Closes: >> https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/ >> Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for >> GEM DMA helpers") > > Thanks, this fixes the issue I was seeing on R-Car Gen3/Gen4 with > rcar-du. > > No regressions on R-Car Gen2 (rcar-du) and R-Mobile A1 (shmobile) > which didn't shown the warning in the first place. Right, your bug report overlapped with the other one. I'll add your report to the patch tags. > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Thanks a lot. Best regards Thomas > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- > geert@linux-m68k.org > > In personal conversations with technical people, I call myself a > hacker. But > when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
On Mon, Jun 17, 2024 at 05:26:37PM +0200, Thomas Zimmermann wrote: > Only export struct fb_info.fix.smem_start if that is required by the > user and the memory does not come from vmalloc(). > > Setting struct fb_info.fix.smem_start breaks systems where DMA > memory is backed by vmalloc address space. An example error is > shown below. > > [ 3.536043] ------------[ cut here ]------------ > [ 3.540716] virt_to_phys used for non-linear address: 000000007fc4f540 (0xffff800086001000) > [ 3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 __virt_to_phys+0x68/0x98 > [ 3.565455] Modules linked in: > [ 3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted 6.6.23-06226-g4986cc3e1b75-dirty #250 > [ 3.577310] Hardware name: NXP i.MX95 19X19 board (DT) > [ 3.582452] Workqueue: events_unbound deferred_probe_work_func > [ 3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 3.595233] pc : __virt_to_phys+0x68/0x98 > [ 3.599246] lr : __virt_to_phys+0x68/0x98 > [ 3.603276] sp : ffff800083603990 > [ 3.677939] Call trace: > [ 3.680393] __virt_to_phys+0x68/0x98 > [ 3.684067] drm_fbdev_dma_helper_fb_probe+0x138/0x238 > [ 3.689214] __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0 > [ 3.695385] drm_fb_helper_initial_config+0x4c/0x68 > [ 3.700264] drm_fbdev_dma_client_hotplug+0x8c/0xe0 > [ 3.705161] drm_client_register+0x60/0xb0 > [ 3.709269] drm_fbdev_dma_setup+0x94/0x148 > > Additionally, DMA memory is assumed to by contiguous in physical > address space, which is not guaranteed by vmalloc(). > > Resolve this by checking the module flag drm_leak_fbdev_smem when > DRM allocated the instance of struct fb_info. Fbdev-dma then only > sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also > guarantee that the framebuffer is not located in vmalloc address > space. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Reported-by: Peng Fan (OSS) <peng.fan@oss.nxp.com> > Closes: https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/ > Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers") > --- > drivers/gpu/drm/drm_fb_helper.c | 6 +++--- > drivers/gpu/drm/drm_fbdev_dma.c | 5 ++++- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index e2e19f49342e1..18565ec684517 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -517,6 +517,9 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper) > if (!info) > return ERR_PTR(-ENOMEM); > > + if (!drm_leak_fbdev_smem) > + info->flags |= FBINFO_HIDE_SMEM_START; > + > ret = fb_alloc_cmap(&info->cmap, 256, 0); > if (ret) > goto err_release; > @@ -1853,9 +1856,6 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper) > info = fb_helper->info; > info->var.pixclock = 0; > > - if (!drm_leak_fbdev_smem) > - info->flags |= FBINFO_HIDE_SMEM_START; > - > /* Need to drop locks to avoid recursive deadlock in > * register_framebuffer. This is ok because the only thing left to do is > * register the fbdev emulation instance in kernel_fb_helper_list. */ > diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c > index 97ef6300d47e0..4b0527ddeb5a7 100644 > --- a/drivers/gpu/drm/drm_fbdev_dma.c > +++ b/drivers/gpu/drm/drm_fbdev_dma.c > @@ -137,7 +137,10 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, > info->flags |= FBINFO_READS_FAST; /* signal caching */ > info->screen_size = sizes->surface_height * fb->pitches[0]; > info->screen_buffer = map.vaddr; > - info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); > + if (!(info->flags & FBINFO_HIDE_SMEM_START)) { I guess since the WARN is conditional on the explicit opt-in with drm_leak_fbdev_smem it should be fine. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer))) > + info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); > + } > info->fix.smem_len = info->screen_size; > > /* deferred I/O */ > -- > 2.45.2 >
Hi Am 18.06.24 um 11:35 schrieb Daniel Vetter: > On Mon, Jun 17, 2024 at 05:26:37PM +0200, Thomas Zimmermann wrote: >> Only export struct fb_info.fix.smem_start if that is required by the >> user and the memory does not come from vmalloc(). >> >> Setting struct fb_info.fix.smem_start breaks systems where DMA >> memory is backed by vmalloc address space. An example error is >> shown below. >> >> [ 3.536043] ------------[ cut here ]------------ >> [ 3.540716] virt_to_phys used for non-linear address: 000000007fc4f540 (0xffff800086001000) >> [ 3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 __virt_to_phys+0x68/0x98 >> [ 3.565455] Modules linked in: >> [ 3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted 6.6.23-06226-g4986cc3e1b75-dirty #250 >> [ 3.577310] Hardware name: NXP i.MX95 19X19 board (DT) >> [ 3.582452] Workqueue: events_unbound deferred_probe_work_func >> [ 3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> [ 3.595233] pc : __virt_to_phys+0x68/0x98 >> [ 3.599246] lr : __virt_to_phys+0x68/0x98 >> [ 3.603276] sp : ffff800083603990 >> [ 3.677939] Call trace: >> [ 3.680393] __virt_to_phys+0x68/0x98 >> [ 3.684067] drm_fbdev_dma_helper_fb_probe+0x138/0x238 >> [ 3.689214] __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0 >> [ 3.695385] drm_fb_helper_initial_config+0x4c/0x68 >> [ 3.700264] drm_fbdev_dma_client_hotplug+0x8c/0xe0 >> [ 3.705161] drm_client_register+0x60/0xb0 >> [ 3.709269] drm_fbdev_dma_setup+0x94/0x148 >> >> Additionally, DMA memory is assumed to by contiguous in physical >> address space, which is not guaranteed by vmalloc(). >> >> Resolve this by checking the module flag drm_leak_fbdev_smem when >> DRM allocated the instance of struct fb_info. Fbdev-dma then only >> sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also >> guarantee that the framebuffer is not located in vmalloc address >> space. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reported-by: Peng Fan (OSS) <peng.fan@oss.nxp.com> >> Closes: https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/ >> Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers") >> --- >> drivers/gpu/drm/drm_fb_helper.c | 6 +++--- >> drivers/gpu/drm/drm_fbdev_dma.c | 5 ++++- >> 2 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index e2e19f49342e1..18565ec684517 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -517,6 +517,9 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper) >> if (!info) >> return ERR_PTR(-ENOMEM); >> >> + if (!drm_leak_fbdev_smem) >> + info->flags |= FBINFO_HIDE_SMEM_START; >> + >> ret = fb_alloc_cmap(&info->cmap, 256, 0); >> if (ret) >> goto err_release; >> @@ -1853,9 +1856,6 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper) >> info = fb_helper->info; >> info->var.pixclock = 0; >> >> - if (!drm_leak_fbdev_smem) >> - info->flags |= FBINFO_HIDE_SMEM_START; >> - >> /* Need to drop locks to avoid recursive deadlock in >> * register_framebuffer. This is ok because the only thing left to do is >> * register the fbdev emulation instance in kernel_fb_helper_list. */ >> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c >> index 97ef6300d47e0..4b0527ddeb5a7 100644 >> --- a/drivers/gpu/drm/drm_fbdev_dma.c >> +++ b/drivers/gpu/drm/drm_fbdev_dma.c >> @@ -137,7 +137,10 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, >> info->flags |= FBINFO_READS_FAST; /* signal caching */ >> info->screen_size = sizes->surface_height * fb->pitches[0]; >> info->screen_buffer = map.vaddr; >> - info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); >> + if (!(info->flags & FBINFO_HIDE_SMEM_START)) { > I guess since the WARN is conditional on the explicit opt-in with > drm_leak_fbdev_smem it should be fine. Yes, that's the intention. If someone enables this flag, it warns if that's not supported. By default, smem_start is clear. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks Best regards Thomas > >> + if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer))) >> + info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); >> + } >> info->fix.smem_len = info->screen_size; >> >> /* deferred I/O */ >> -- >> 2.45.2 >>
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e2e19f49342e1..18565ec684517 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -517,6 +517,9 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper) if (!info) return ERR_PTR(-ENOMEM); + if (!drm_leak_fbdev_smem) + info->flags |= FBINFO_HIDE_SMEM_START; + ret = fb_alloc_cmap(&info->cmap, 256, 0); if (ret) goto err_release; @@ -1853,9 +1856,6 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper) info = fb_helper->info; info->var.pixclock = 0; - if (!drm_leak_fbdev_smem) - info->flags |= FBINFO_HIDE_SMEM_START; - /* Need to drop locks to avoid recursive deadlock in * register_framebuffer. This is ok because the only thing left to do is * register the fbdev emulation instance in kernel_fb_helper_list. */ diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c index 97ef6300d47e0..4b0527ddeb5a7 100644 --- a/drivers/gpu/drm/drm_fbdev_dma.c +++ b/drivers/gpu/drm/drm_fbdev_dma.c @@ -137,7 +137,10 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper, info->flags |= FBINFO_READS_FAST; /* signal caching */ info->screen_size = sizes->surface_height * fb->pitches[0]; info->screen_buffer = map.vaddr; - info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); + if (!(info->flags & FBINFO_HIDE_SMEM_START)) { + if (!drm_WARN_ON(dev, is_vmalloc_addr(info->screen_buffer))) + info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer)); + } info->fix.smem_len = info->screen_size; /* deferred I/O */
Only export struct fb_info.fix.smem_start if that is required by the user and the memory does not come from vmalloc(). Setting struct fb_info.fix.smem_start breaks systems where DMA memory is backed by vmalloc address space. An example error is shown below. [ 3.536043] ------------[ cut here ]------------ [ 3.540716] virt_to_phys used for non-linear address: 000000007fc4f540 (0xffff800086001000) [ 3.552628] WARNING: CPU: 4 PID: 61 at arch/arm64/mm/physaddr.c:12 __virt_to_phys+0x68/0x98 [ 3.565455] Modules linked in: [ 3.568525] CPU: 4 PID: 61 Comm: kworker/u12:5 Not tainted 6.6.23-06226-g4986cc3e1b75-dirty #250 [ 3.577310] Hardware name: NXP i.MX95 19X19 board (DT) [ 3.582452] Workqueue: events_unbound deferred_probe_work_func [ 3.588291] pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 3.595233] pc : __virt_to_phys+0x68/0x98 [ 3.599246] lr : __virt_to_phys+0x68/0x98 [ 3.603276] sp : ffff800083603990 [ 3.677939] Call trace: [ 3.680393] __virt_to_phys+0x68/0x98 [ 3.684067] drm_fbdev_dma_helper_fb_probe+0x138/0x238 [ 3.689214] __drm_fb_helper_initial_config_and_unlock+0x2b0/0x4c0 [ 3.695385] drm_fb_helper_initial_config+0x4c/0x68 [ 3.700264] drm_fbdev_dma_client_hotplug+0x8c/0xe0 [ 3.705161] drm_client_register+0x60/0xb0 [ 3.709269] drm_fbdev_dma_setup+0x94/0x148 Additionally, DMA memory is assumed to by contiguous in physical address space, which is not guaranteed by vmalloc(). Resolve this by checking the module flag drm_leak_fbdev_smem when DRM allocated the instance of struct fb_info. Fbdev-dma then only sets smem_start only if required (via FBINFO_HIDE_SMEM_START). Also guarantee that the framebuffer is not located in vmalloc address space. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Reported-by: Peng Fan (OSS) <peng.fan@oss.nxp.com> Closes: https://lore.kernel.org/dri-devel/20240604080328.4024838-1-peng.fan@oss.nxp.com/ Fixes: b79fe9abd58b ("drm/fbdev-dma: Implement fbdev emulation for GEM DMA helpers") --- drivers/gpu/drm/drm_fb_helper.c | 6 +++--- drivers/gpu/drm/drm_fbdev_dma.c | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-)