<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 20, 2012 at 12:49 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"><div><div>On 11/20/12, Prathyush K <<a href="mailto:prathyush@chromium.org" target="_blank">prathyush@chromium.org</a>> wrote:<br>
> On Mon, Nov 19, 2012 at 3:14 PM, Kyungmin Park <<a href="mailto:kmpark@infradead.org" target="_blank">kmpark@infradead.org</a>><br>
> wrote:<br>
><br>
>> Hi,<br>
>><br>
>> On 11/15/12, Prathyush K <<a href="mailto:prathyush.k@samsung.com" target="_blank">prathyush.k@samsung.com</a>> wrote:<br>
>> > The 'pages' structure is not required since we can use the 'sgt'. Even<br>
>> for<br>
>> > CONTIG buffers, a SGT is created (which will have just one sgl). This<br>
>> > SGT<br>
>> > can be used during mmap instead of 'pages'. The 'page_size' element of<br>
>> the<br>
>> > structure is also not used anywhere and is removed.<br>
>> > This patch also fixes a memory leak where the 'pages' structure was<br>
>> > 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" target="_blank">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<br>
>> > 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<br>
>> > *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<br>
>> 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>
>> It's never happned by for_each_sg definition.<br>
>><br>
>> Agreed. This normally should never happen.<br>
><br>
> But actually this is existing code. I have not changed this.<br>
> I had just moved the above code from inside a 'if else' condition to<br>
> outside.<br>
</div></div>then you can remove the redundent codes.<br></blockquote><div><br></div><div>Ok.<br></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>><br>
><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>
>> > - pfn = page_to_pfn(buf->pages[0]) + page_offset;<br>
>> > + if (i >= buf->sgt->nents) {<br>
>> ditto. IOW it's dead code.<br>
>><br>
>> Again, this is also existing code.<br>
><br>
> But this error can actually happen if the page offset is not valid.<br>
> e.g. if page_offset > (buf_size >> PAGE_SHIFT)<br>
> In this case, the loop 'for_each_sg' will never break in between<br>
> and 'i' will be equal to nents. This check will return error which<br>
> is correct.<br>
</div>No, Even though page_offset greater than (buf_size >> PAGE_SHIFT),<br>
the 'i' is checked at for_each_sg, 'i' can't exceed than '(nr)'.<br>
<br>
#define for_each_sg(sglist, sg, nr, __i) \<br>
for (__i = 0, sg = (sglist); __i < (nr); __i++, sg = sg_next(sg))<br>
<div><br></div></blockquote><div>Yes, 'i' cannot exceed 'nr' inside the loop.</div><div>But this check is outside the loop.<br></div><div><br></div><div>If the page offset is greater than the maximum number of pages present<br>
</div><div>in the sgt, the loop will never break.</div><div>So outside the loop 'i' will be EQUAL to 'nr'.</div><div>This is what we are checking for.</div><div><br></div><div>I think to avoid confusion, a better thing is to check for validity of page_offset</div>
<div>at the beginning itself.</div><div><br></div><div><div> if (page_offset >= (buf->size >> PAGE_SHIFT)) {</div><div> DRM_ERROR("invalid page offset\n");</div><div> return -EINVAL;</div>
<div> }</div></div><div><br></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>
><br>
> Thanks for reviewing. If required, I can post another patch<br>
> to remove the first redundant check. But I don't think it should<br>
> be part of this patch.<br>
<br>
</div>Please send the updated patch. no need addition patch for it.<br>
<br></blockquote><div><br></div><div>Sure. I shall send across the updated patch asap.</div><div>Thanks for the review comments.<br></div><div><br></div><div>Prathyush</div><div><br></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">
Thank you,<br>
Kyungmin Park<br>
<div><div>><br>
><br>
>> Thank you,<br>
>> Kyungmin Park<br>
>> > + 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" target="_blank">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" target="_blank">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>
</div></div></blockquote></div><br></div>