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

[Prathyush K <prathyush@xxxxxxxxxxxx>]

On Mon, Nov 19, 2012 at 3:14 PM, Kyungmin Park <kmpark@xxxxxxxxxxxxx> wrote:

Hi,

On 11/15/12, Prathyush K <prathyush.k@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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.

 

> +                     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.

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.

 

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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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