<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Nov 19, 2012 at 3:14 PM, Kyungmin Park <span dir="ltr"><<a href="mailto:kmpark@infradead.org" target="_blank">kmpark@infradead.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<div><div class="h5"><br>
On 11/15/12, Prathyush K <<a href="mailto:prathyush.k@samsung.com">prathyush.k@samsung.com</a>> wrote:<br>
> The 'pages' structure is not required since we can use the 'sgt'. Even for<br>
> CONTIG buffers, a SGT is created (which will have just one sgl). This SGT<br>
> can be used during mmap instead of 'pages'. The 'page_size' element of the<br>
> structure is also not used anywhere and is removed.<br>
> This patch also fixes a memory leak where the 'pages' structure was being<br>
> allocated during gem buffer allocation but not being freed during<br>
> deallocate.<br>
><br>
> Signed-off-by: Prathyush K <<a href="mailto:prathyush.k@samsung.com">prathyush.k@samsung.com</a>><br>
> ---<br>
> drivers/gpu/drm/exynos/exynos_drm_buf.c | 20 --------------<br>
> drivers/gpu/drm/exynos/exynos_drm_buf.h | 4 +-<br>
> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 3 +-<br>
> drivers/gpu/drm/exynos/exynos_drm_gem.c | 39<br>
> +++++++++++----------------<br>
> drivers/gpu/drm/exynos/exynos_drm_gem.h | 4 ---<br>
> 5 files changed, 19 insertions(+), 51 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.c<br>
> b/drivers/gpu/drm/exynos/exynos_drm_buf.c<br>
> index 48c5896..72bf97b 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_buf.c<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_buf.c<br>
> @@ -34,8 +34,6 @@ static int lowlevel_buffer_allocate(struct drm_device<br>
> *dev,<br>
> unsigned int flags, struct exynos_drm_gem_buf *buf)<br>
> {<br>
> int ret = 0;<br>
> - unsigned int npages, i = 0;<br>
> - struct scatterlist *sgl;<br>
> enum dma_attr attr = DMA_ATTR_FORCE_CONTIGUOUS;<br>
><br>
> DRM_DEBUG_KMS("%s\n", __FILE__);<br>
> @@ -73,22 +71,6 @@ static int lowlevel_buffer_allocate(struct drm_device<br>
> *dev,<br>
> goto err_free_sgt;<br>
> }<br>
><br>
> - npages = buf->sgt->nents;<br>
> -<br>
> - buf->pages = kzalloc(sizeof(struct page) * npages, GFP_KERNEL);<br>
> - if (!buf->pages) {<br>
> - DRM_ERROR("failed to allocate pages.\n");<br>
> - ret = -ENOMEM;<br>
> - goto err_free_table;<br>
> - }<br>
> -<br>
> - sgl = buf->sgt->sgl;<br>
> - while (i < npages) {<br>
> - buf->pages[i] = sg_page(sgl);<br>
> - sgl = sg_next(sgl);<br>
> - i++;<br>
> - }<br>
> -<br>
> DRM_DEBUG_KMS("vaddr(0x%lx), dma_addr(0x%lx), size(0x%lx)\n",<br>
> (unsigned long)buf->kvaddr,<br>
> (unsigned long)buf->dma_addr,<br>
> @@ -96,8 +78,6 @@ static int lowlevel_buffer_allocate(struct drm_device<br>
> *dev,<br>
><br>
> return ret;<br>
><br>
> -err_free_table:<br>
> - sg_free_table(buf->sgt);<br>
> err_free_sgt:<br>
> kfree(buf->sgt);<br>
> buf->sgt = NULL;<br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.h<br>
> b/drivers/gpu/drm/exynos/exynos_drm_buf.h<br>
> index 3388e4e..25cf162 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_buf.h<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_buf.h<br>
> @@ -34,12 +34,12 @@ struct exynos_drm_gem_buf *exynos_drm_init_buf(struct<br>
> drm_device *dev,<br>
> void exynos_drm_fini_buf(struct drm_device *dev,<br>
> struct exynos_drm_gem_buf *buffer);<br>
><br>
> -/* allocate physical memory region and setup sgt and pages. */<br>
> +/* allocate physical memory region and setup sgt. */<br>
> int exynos_drm_alloc_buf(struct drm_device *dev,<br>
> struct exynos_drm_gem_buf *buf,<br>
> unsigned int flags);<br>
><br>
> -/* release physical memory region, sgt and pages. */<br>
> +/* release physical memory region, and sgt. */<br>
> void exynos_drm_free_buf(struct drm_device *dev,<br>
> unsigned int flags,<br>
> struct exynos_drm_gem_buf *buffer);<br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c<br>
> b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c<br>
> index b98da30..615a049 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c<br>
> @@ -93,8 +93,7 @@ static struct sg_table *<br>
> goto err_unlock;<br>
> }<br>
><br>
> - DRM_DEBUG_PRIME("buffer size = 0x%lx page_size = 0x%lx\n",<br>
> - buf->size, buf->page_size);<br>
> + DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);<br>
><br>
> err_unlock:<br>
> mutex_unlock(&dev->struct_mutex);<br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c<br>
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c<br>
> index 50d73f1..40999ac 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c<br>
> @@ -99,34 +99,27 @@ static int exynos_drm_gem_map_buf(struct drm_gem_object<br>
> *obj,<br>
> unsigned long pfn;<br>
> int i;<br>
><br>
> - if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG) {<br>
> - if (!buf->sgt)<br>
> - return -EINTR;<br>
> -<br>
> - sgl = buf->sgt->sgl;<br>
> - for_each_sg(buf->sgt->sgl, sgl, buf->sgt->nents, i) {<br>
> - if (!sgl) {<br>
> - DRM_ERROR("invalid SG table\n");<br>
> - return -EINTR;<br>
> - }<br>
> - if (page_offset < (sgl->length >> PAGE_SHIFT))<br>
> - break;<br>
> - page_offset -= (sgl->length >> PAGE_SHIFT);<br>
> - }<br>
> -<br>
> - if (i >= buf->sgt->nents) {<br>
> - DRM_ERROR("invalid page offset\n");<br>
> - return -EINVAL;<br>
> - }<br>
> + if (!buf->sgt)<br>
> + return -EINTR;<br>
><br>
> - pfn = __phys_to_pfn(sg_phys(sgl)) + page_offset;<br>
> - } else {<br>
> - if (!buf->pages)<br>
> + sgl = buf->sgt->sgl;<br>
> + for_each_sg(buf->sgt->sgl, sgl, buf->sgt->nents, i) {<br>
> + if (!sgl) {<br>
</div></div>It's never happned by for_each_sg definition.<br>
<div class="im"><br></div></blockquote><div>Agreed. This normally should never happen.<br></div><div><br></div><div>But actually this is existing code. I have not changed this.</div><div>I had just moved the above code from inside a 'if else' condition to outside.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
> + DRM_ERROR("invalid SG table\n");<br>
> return -EINTR;<br>
> + }<br>
> + if (page_offset < (sgl->length >> PAGE_SHIFT))<br>
> + break;<br>
> + page_offset -= (sgl->length >> PAGE_SHIFT);<br>
> + }<br>
><br>
> - pfn = page_to_pfn(buf->pages[0]) + page_offset;<br>
> + if (i >= buf->sgt->nents) {<br>
</div>ditto. IOW it's dead code.<br>
<br></blockquote><div>Again, this is also existing code.</div><div><br></div><div>But this error can actually happen if the page offset is not valid.</div><div>e.g. if page_offset > (buf_size >> PAGE_SHIFT)</div>
<div>In this case, the loop 'for_each_sg' will never break in between</div><div>and 'i' will be equal to nents. This check will return error which</div><div>is correct.</div><div><br></div><div>Thanks for reviewing. If required, I can post another patch</div>
<div>to remove the first redundant check. But I don't think it should</div><div>be part of this patch.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Thank you,<br>
Kyungmin Park<br>
<div class=""><div class="h5">> + DRM_ERROR("invalid page offset\n");<br>
> + return -EINVAL;<br>
> }<br>
><br>
> + pfn = __phys_to_pfn(sg_phys(sgl)) + page_offset;<br>
> +<br>
> return vm_insert_mixed(vma, f_vaddr, pfn);<br>
> }<br>
><br>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h<br>
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h<br>
> index 83d21ef..3600b3b 100644<br>
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h<br>
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h<br>
> @@ -39,8 +39,6 @@<br>
> * - this address could be physical address without IOMMU and<br>
> * device address with IOMMU.<br>
> * @sgt: sg table to transfer page data.<br>
> - * @pages: contain all pages to allocated memory region.<br>
> - * @page_size: could be 4K, 64K or 1MB.<br>
> * @size: size of allocated memory region.<br>
> */<br>
> struct exynos_drm_gem_buf {<br>
> @@ -48,8 +46,6 @@ struct exynos_drm_gem_buf {<br>
> dma_addr_t dma_addr;<br>
> struct dma_attrs dma_attrs;<br>
> struct sg_table *sgt;<br>
> - struct page **pages;<br>
> - unsigned long page_size;<br>
> unsigned long size;<br>
> };<br>
><br>
> --<br>
> 1.7.0.4<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>
><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>
</div></div></blockquote></div><br></div>