<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2013/5/5 Tomasz Figa <span dir="ltr"><<a href="mailto:tomasz.figa@gmail.com" target="_blank">tomasz.figa@gmail.com</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi,<br>
<br>
Recently I've been working a bit on a DRM driver for the GPU of Samsung<br>
S3C6410 SoCs, which required me to familiarize a bit with exynos-drm, as<br>
it already contains a KMS driver which is compatible with the SoC I'm<br>
working with, making it a good place to put my driver in.<br>
<br>
Reading through exynos_drm_buf.c I stumbled across following (a bit long)<br>
piece of code:<br>
<br>
dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &buf->dma_attrs);<br>
<br>
nr_pages = buf->size >> PAGE_SHIFT;<br>
<br>
if (!is_drm_iommu_supported(dev)) {<br>
dma_addr_t start_addr;<br>
unsigned int i = 0;<br>
<br>
buf->pages = kzalloc(sizeof(struct page) * nr_pages,<br>
GFP_KERNEL);<br>
if (!buf->pages) {<br>
DRM_ERROR("failed to allocate pages.\n");<br>
return -ENOMEM;<br>
}<br>
<br>
buf->kvaddr = dma_alloc_attrs(dev->dev, buf->size,<br>
&buf->dma_addr, GFP_KERNEL,<br>
&buf->dma_attrs);<br>
if (!buf->kvaddr) {<br>
DRM_ERROR("failed to allocate buffer.\n");<br>
kfree(buf->pages);<br>
return -ENOMEM;<br>
}<br>
<br>
start_addr = buf->dma_addr;<br>
while (i < nr_pages) {<br>
buf->pages[i] = phys_to_page(start_addr);<br>
start_addr += PAGE_SIZE;<br>
i++;<br>
}<br>
} else {<br>
<br>
buf->pages = dma_alloc_attrs(dev->dev, buf->size,<br>
&buf->dma_addr, GFP_KERNEL,<br>
&buf->dma_attrs);<br>
if (!buf->pages) {<br>
DRM_ERROR("failed to allocate buffer.\n");<br>
return -ENOMEM;<br>
}<br>
}<br>
<br>
buf->sgt = drm_prime_pages_to_sg(buf->pages, nr_pages);<br>
if (!buf->sgt) {<br>
DRM_ERROR("failed to get sg table.\n");<br>
ret = -ENOMEM;<br>
goto err_free_attrs;<br>
}<br>
<br>
Notice that the value returned by dma_alloc_attrs() is assumed by this<br>
code to be either a kernel virtual address (in !is_drm_iommu_supported()<br>
case) or struct pages ** (in is_drm_iommu_supported() case), which seemed<br>
a bit worrying to me, making me do a more in depth research on how<br>
dma_alloc_attrs works.<br>
<br>
This, in turn, led me to following excerpt of Documentation/DMA-<br>
attributes.txt :<br>
<br>
DMA_ATTR_NO_KERNEL_MAPPING<br>
--------------------------<br>
<br>
DMA_ATTR_NO_KERNEL_MAPPING lets the platform to avoid creating a kernel<br>
virtual mapping for the allocated buffer. On some architectures creating<br>
such mapping is non-trivial task and consumes very limited resources<br>
(like kernel virtual address space or dma consistent address space).<br>
Buffers allocated with this attribute can be only passed to user space<br>
by calling dma_mmap_attrs(). By using this API, you are guaranteeing<br>
that you won't dereference the pointer returned by dma_alloc_attr(). You<br>
can threat it as a cookie that must be passed to dma_mmap_attrs() and<br>
dma_free_attrs(). Make sure that both of these also get this attribute<br>
set on each call.<br>
<br>
of which the following sentence is the most relevant:<br>
<br>
By using this API, you are guaranteeing that you won't dereference the<br>
pointer returned by dma_alloc_attr().<br>
<br>
This statement is obviously ignored by buffer allocation code of exynos-<br>
drm.<br>
<br>
A simple git blame revealed that this brokenness has been introduced by<br>
commit:<br>
<br>
4744ad2 drm/exynos: use DMA_ATTR_NO_KERNEL_MAPPING attribute<br>
<br>
<br>
and later fixed a bit by following depending patches (because the original<br>
patch apparently broke any allocations without IOMMU):<br>
<br>
c704f1b drm/exynos: consider no iommu support to console framebuffer<br>
694be45 drm/exynos: consider buffer allocation without iommu<br>
<br>
<br>
Now, the real question is whether my reasoning is incorrect (sorry for the<br>
noise then) or this is really a bug which needs to be fixed?<br>
<br></blockquote><div><br></div><div>Hi Tomasz,<br><br>Your question is reasonable to me. And below is my opinion,<br><br>First, also the below attribute says like below,<br><br>DMA_ATTR_NO_KERNEL_MAPPING<br>--------------------------<br>
...<br>Since it is optional for platforms to implement<br>DMA_ATTR_NO_KERNEL_MAPPING, those that do not will simply ignore the<br>attribute and exhibit default behavior.<br><br></div><div><br></div><div>In case of ARM SoC, it seems like that it just ignores the attribute without iommu: in case of no iommu, dma_alloc_attr() always maps pages allocated from highmem with kernel space. So I think we make sure that exynos drm driver sets the attribute only with iommu to avoid such confusing. For this, will post it soon.<br>
</div><div> <br></div><div>Thanks,<br>Inki Dae<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Best regards,<br>
Tomasz<br>
<br>
_______________________________________________<br>
dri-devel mailing list<br>
<a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
</blockquote></div><br></div></div>