| Message ID | 51A8B220.6070308@monstr.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 31 May 2013 16:22:24 Michal Simek wrote: > @@ -307,7 +319,11 @@ static int xilinxfb_assign(struct platform_device *pdev, > > /* Fill struct fb_info */ > drvdata->info.device = dev; > - drvdata->info.screen_base = (void __iomem *)drvdata->fb_virt; > + if (drvdata->fb_virt) > + drvdata->info.screen_base = (__force void __iomem *) > + drvdata->fb_virt; > + else > + drvdata->info.screen_base = drvdata->fb_virt_io; Yes, unfortunately, this is what all other frame buffer drivers do at the moment. It is technically not correct, but most architectures are able to call readl/writel on regular memory, or dereference __iomem tokens, so we often get away with it. It's probably not worth fixing it in the fbdev code base as that would be a huge change, and people are migrating to DRM/KMS. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/31/2013 09:56 AM, Arnd Bergmann wrote: > Yes, unfortunately, this is what all other frame buffer drivers do > at the moment. It is technically not correct, but most architectures > are able to call readl/writel on regular memory, or dereference > __iomem tokens, so we often get away with it. It's probably not > worth fixing it in the fbdev code base as that would be a huge > change, and people are migrating to DRM/KMS. But why bother fixing this bug if it just makes things worse? Sparse is supposed to warn us about bad code. This patch doesn't fix the bug, it just masks the warnings!
On Friday 31 May 2013 16:22:24 Michal Simek wrote: > if (pdata->fb_phys) { > drvdata->fb_phys = pdata->fb_phys; > - drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize); > + drvdata->fb_virt_io = ioremap(pdata->fb_phys, fbsize); > + > + if (!drvdata->fb_virt_io) { > + dev_err(dev, "Could not allocate frame buffer memory\n"); > + rc = -ENOMEM; > + if (drvdata->flags & BUS_ACCESS_FLAG) > + goto err_fbmem; > + else > + goto err_region; > + } > + > + /* Clear (turn to black) the framebuffer */ > + memset_io(drvdata->fb_virt_io, 0, fbsize); > } else { > - drvdata->fb_alloced = 1; > drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize), > &drvdata->fb_phys, GFP_KERNEL); > - } > I think you also want to use ioremap_wc or dma_alloc_writecombine here, to get a write-combining mapping, rather than a device mapping that you would use for MMIO register access. There is also a builtin assumption in the code above that the DMA address space pointer (which you pass into REG_FB_ADDR) is the same as what you pass into drvdata->info.fix.smem_start. That is not the case in general, but I don't see a good way around it when pdata->fb_phys is set by the platform to something outside of system memory. It should probably have a comment next to it. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 31 May 2013 10:06:43 Timur Tabi wrote: > On 05/31/2013 09:56 AM, Arnd Bergmann wrote: > > Yes, unfortunately, this is what all other frame buffer drivers do > > at the moment. It is technically not correct, but most architectures > > are able to call readl/writel on regular memory, or dereference > > __iomem tokens, so we often get away with it. It's probably not > > worth fixing it in the fbdev code base as that would be a huge > > change, and people are migrating to DRM/KMS. > > But why bother fixing this bug if it just makes things worse? Sparse is > supposed to warn us about bad code. This patch doesn't fix the bug, it > just masks the warnings! Yes, good point. It's probably best cast the ioremap() output to a regular pointer here, as that is actually just uncached RAM, not an MMIO register. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/31/2013 05:29 PM, Arnd Bergmann wrote: > On Friday 31 May 2013 10:06:43 Timur Tabi wrote: >> On 05/31/2013 09:56 AM, Arnd Bergmann wrote: >>> Yes, unfortunately, this is what all other frame buffer drivers do >>> at the moment. It is technically not correct, but most architectures >>> are able to call readl/writel on regular memory, or dereference >>> __iomem tokens, so we often get away with it. It's probably not >>> worth fixing it in the fbdev code base as that would be a huge >>> change, and people are migrating to DRM/KMS. >> >> But why bother fixing this bug if it just makes things worse? Sparse is >> supposed to warn us about bad code. This patch doesn't fix the bug, it >> just masks the warnings! > > Yes, good point. It's probably best cast the ioremap() output to > a regular pointer here, as that is actually just uncached RAM, > not an MMIO register. ok. It means I will just remove this patch from this patchset. Thanks, Michal
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c index f3d4a69..885f294 100644 --- a/drivers/video/xilinxfb.c +++ b/drivers/video/xilinxfb.c @@ -132,8 +132,8 @@ struct xilinxfb_drvdata { unsigned int dcr_len; #endif void *fb_virt; /* virt. address of the frame buffer */ + void __iomem *fb_virt_io; /* virt. address of the frame buffer */ dma_addr_t fb_phys; /* phys. address of the frame buffer */ - int fb_alloced; /* Flag, was the fb memory alloced? */ u8 flags; /* features of the driver */ @@ -270,24 +270,36 @@ static int xilinxfb_assign(struct platform_device *pdev, /* Allocate the framebuffer memory */ if (pdata->fb_phys) { drvdata->fb_phys = pdata->fb_phys; - drvdata->fb_virt = ioremap(pdata->fb_phys, fbsize); + drvdata->fb_virt_io = ioremap(pdata->fb_phys, fbsize); + + if (!drvdata->fb_virt_io) { + dev_err(dev, "Could not allocate frame buffer memory\n"); + rc = -ENOMEM; + if (drvdata->flags & BUS_ACCESS_FLAG) + goto err_fbmem; + else + goto err_region; + } + + /* Clear (turn to black) the framebuffer */ + memset_io(drvdata->fb_virt_io, 0, fbsize); } else { - drvdata->fb_alloced = 1; drvdata->fb_virt = dma_alloc_coherent(dev, PAGE_ALIGN(fbsize), &drvdata->fb_phys, GFP_KERNEL); - } - if (!drvdata->fb_virt) { - dev_err(dev, "Could not allocate frame buffer memory\n"); - rc = -ENOMEM; - if (drvdata->flags & BUS_ACCESS_FLAG) - goto err_fbmem; - else - goto err_region; + if (!drvdata->fb_virt_io) { + dev_err(dev, "Could not allocate frame buffer memory\n"); + rc = -ENOMEM; + if (drvdata->flags & BUS_ACCESS_FLAG) + goto err_fbmem; + else + goto err_region; + memset(drvdata->fb_virt, 0, fbsize); } - /* Clear (turn to black) the framebuffer */ - memset_io((void __iomem *)drvdata->fb_virt, 0, fbsize); + /* Tell the hardware where the frame buffer is */ xilinx_fb_out32(drvdata, REG_FB_ADDR, drvdata->fb_phys); @@ -307,7 +319,11 @@ static int xilinxfb_assign(struct platform_device *pdev,