| Message ID | 1352975149-29108-1-git-send-email-prathyush.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > + 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. 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 >
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. > > + 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@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 >
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 >> >
On Tue, Nov 20, 2012 at 12:49 PM, Kyungmin Park <kmpark@infradead.org>wrote: > 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. > Ok. > > > > > >> > + 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)) > > Yes, 'i' cannot exceed 'nr' inside the loop. But this check is outside the loop. If the page offset is greater than the maximum number of pages present in the sgt, the loop will never break. So outside the loop 'i' will be EQUAL to 'nr'. This is what we are checking for. I think to avoid confusion, a better thing is to check for validity of page_offset at the beginning itself. if (page_offset >= (buf->size >> PAGE_SHIFT)) { DRM_ERROR("invalid page offset\n"); return -EINVAL; } > > > > 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. > > Sure. I shall send across the updated patch asap. Thanks for the review comments. Prathyush > 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 > >> > > >
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) { + 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) { + 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; };
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(-)