Re: [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer

On 11/20/12, Prathyush K prathyush@chromium.org wrote:

On Mon, Nov 19, 2012 at 3:14 PM, Kyungmin Park kmpark@infradead.org wrote:

...

Hi,

On 11/15/12, Prathyush K prathyush.k@samsung.com wrote:

...

The 'pages' structure is not required since we can use the 'sgt'. Even

for

...

CONTIG buffers, a SGT is created (which will have just one sgl). This SGT can be used during mmap instead of 'pages'. The 'page_size' element of

the

...

structure is also not used anywhere and is removed. This patch also fixes a memory leak where the 'pages' structure was being allocated during gem buffer allocation but not being freed during deallocate.

Signed-off-by: Prathyush K prathyush.k@samsung.com

drivers/gpu/drm/exynos/exynos_drm_buf.c | 20 -------------- drivers/gpu/drm/exynos/exynos_drm_buf.h | 4 +- drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 3 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 39 +++++++++++---------------- drivers/gpu/drm/exynos/exynos_drm_gem.h | 4 --- 5 files changed, 19 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.c b/drivers/gpu/drm/exynos/exynos_drm_buf.c index 48c5896..72bf97b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_buf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_buf.c @@ -34,8 +34,6 @@ static int lowlevel_buffer_allocate(struct drm_device *dev, unsigned int flags, struct exynos_drm_gem_buf *buf) { int ret = 0;

• unsigned int npages, i = 0;
  

• struct scatterlist *sgl;
  enum dma_attr attr = DMA_ATTR_FORCE_CONTIGUOUS;
  
  DRM_DEBUG_KMS("%s\n", __FILE__);
  

@@ -73,22 +71,6 @@ static int lowlevel_buffer_allocate(struct drm_device *dev, goto err_free_sgt; }

• npages = buf->sgt->nents;
  

• buf->pages = kzalloc(sizeof(struct page) * npages, GFP_KERNEL);
  

• if (!buf->pages) {
  

•         DRM_ERROR("failed to allocate pages.\n");
  

•         ret = -ENOMEM;
  

•         goto err_free_table;
  

• }
  

• sgl = buf->sgt->sgl;
  

• while (i < npages) {
  

•         buf->pages[i] = sg_page(sgl);
  

•         sgl = sg_next(sgl);
  

•         i++;
  

• }
  

• DRM_DEBUG_KMS("vaddr(0x%lx), dma_addr(0x%lx), size(0x%lx)\n",
                  (unsigned long)buf->kvaddr,
                  (unsigned long)buf->dma_addr,
  

@@ -96,8 +78,6 @@ static int lowlevel_buffer_allocate(struct drm_device *dev,

  return ret;

-err_free_table:

• sg_free_table(buf->sgt);
  

err_free_sgt: kfree(buf->sgt); buf->sgt = NULL; diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.h b/drivers/gpu/drm/exynos/exynos_drm_buf.h index 3388e4e..25cf162 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_buf.h +++ b/drivers/gpu/drm/exynos/exynos_drm_buf.h @@ -34,12 +34,12 @@ struct exynos_drm_gem_buf *exynos_drm_init_buf(struct drm_device *dev, void exynos_drm_fini_buf(struct drm_device *dev, struct exynos_drm_gem_buf *buffer);

-/* allocate physical memory region and setup sgt and pages. */ +/* allocate physical memory region and setup sgt. */ int exynos_drm_alloc_buf(struct drm_device *dev, struct exynos_drm_gem_buf *buf, unsigned int flags);

-/* release physical memory region, sgt and pages. */ +/* release physical memory region, and sgt. */ void exynos_drm_free_buf(struct drm_device *dev, unsigned int flags, struct exynos_drm_gem_buf *buffer); diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c index b98da30..615a049 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c @@ -93,8 +93,7 @@ static struct sg_table * goto err_unlock; }

• DRM_DEBUG_PRIME("buffer size = 0x%lx page_size = 0x%lx\n",
  

•                 buf->size, buf->page_size);
  

• DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);
  
  

err_unlock: mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c index 50d73f1..40999ac 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c @@ -99,34 +99,27 @@ static int exynos_drm_gem_map_buf(struct

drm_gem_object

...

*obj, unsigned long pfn; int i;

• if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG) {
  

•         if (!buf->sgt)
  

•                 return -EINTR;
  

•         sgl = buf->sgt->sgl;
  

•         for_each_sg(buf->sgt->sgl, sgl, buf->sgt->nents, i) {
  

•                 if (!sgl) {
  

•                         DRM_ERROR("invalid SG table\n");
  

•                         return -EINTR;
  

•                 }
  

•                 if (page_offset < (sgl->length >> PAGE_SHIFT))
  

•                         break;
  

•                 page_offset -=  (sgl->length >> PAGE_SHIFT);
  

•         }
  

•         if (i >= buf->sgt->nents) {
  

•                 DRM_ERROR("invalid page offset\n");
  

•                 return -EINVAL;
  

•         }
  

• if (!buf->sgt)
  

•         return -EINTR;
  
  

•         pfn = __phys_to_pfn(sg_phys(sgl)) + page_offset;
  

• } else {
  

•         if (!buf->pages)
  

• sgl = buf->sgt->sgl;
  

• for_each_sg(buf->sgt->sgl, sgl, buf->sgt->nents, i) {
  

•         if (!sgl) {
  

It's never happned by for_each_sg definition.

Agreed. This normally should never happen.

But actually this is existing code. I have not changed this. I had just moved the above code from inside a 'if else' condition to outside.

then you can remove the redundent codes.

...

...

•                 DRM_ERROR("invalid SG table\n");
                  return -EINTR;
  

•         }
  

•         if (page_offset < (sgl->length >> PAGE_SHIFT))
  

•                 break;
  

•         page_offset -=  (sgl->length >> PAGE_SHIFT);
  

• }
  
  

•         pfn = page_to_pfn(buf->pages[0]) + page_offset;
  

• if (i >= buf->sgt->nents) {
  

ditto. IOW it's dead code.

Again, this is also existing code.

But this error can actually happen if the page offset is not valid. e.g. if page_offset > (buf_size >> PAGE_SHIFT) In this case, the loop 'for_each_sg' will never break in between and 'i' will be equal to nents. This check will return error which is correct.

No, Even though page_offset greater than (buf_size >> PAGE_SHIFT), the 'i' is checked at for_each_sg, 'i' can't exceed than '(nr)'.

#define for_each_sg(sglist, sg, nr, __i) \ for (__i = 0, sg = (sglist); __i < (nr); __i++, sg = sg_next(sg))

Thanks for reviewing. If required, I can post another patch to remove the first redundant check. But I don't think it should be part of this patch.

Please send the updated patch. no need addition patch for it.

Thank you, Kyungmin Park

...

Thank you, Kyungmin Park

...

•         DRM_ERROR("invalid page offset\n");
  

•         return -EINVAL;
  }
  
  

• pfn = __phys_to_pfn(sg_phys(sgl)) + page_offset;
  

• return vm_insert_mixed(vma, f_vaddr, pfn);
  

}

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h index 83d21ef..3600b3b 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h @@ -39,8 +39,6 @@

• • this address could be physical address without IOMMU and
• device address with IOMMU.
• @sgt: sg table to transfer page data.

• • @pages: contain all pages to allocated memory region.
• • @page_size: could be 4K, 64K or 1MB.
  • @size: size of allocated memory region.
  */

struct exynos_drm_gem_buf { @@ -48,8 +46,6 @@ struct exynos_drm_gem_buf { dma_addr_t dma_addr; struct dma_attrs dma_attrs; struct sg_table *sgt;

• struct page             **pages;
  

• unsigned long           page_size;
  unsigned long           size;
  

};

-- 1.7.0.4

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel