[PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
Kyungmin Park
kmpark at infradead.org
Mon Nov 19 23:19:51 PST 2012
On 11/20/12, Prathyush K <prathyush at chromium.org> wrote:
> On Mon, Nov 19, 2012 at 3:14 PM, Kyungmin Park <kmpark at infradead.org>
> wrote:
>
>> Hi,
>>
>> On 11/15/12, Prathyush K <prathyush.k at 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 at 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 at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
More information about the dri-devel
mailing list