All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
@ 2012-11-15 10:25 Prathyush K
  2012-11-19  9:44 ` Kyungmin Park
  0 siblings, 1 reply; 5+ messages in thread
From: Prathyush K @ 2012-11-15 10:25 UTC (permalink / raw)
  To: dri-devel

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) {
+			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;
 };
 
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
  2012-11-15 10:25 [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer Prathyush K
@ 2012-11-19  9:44 ` Kyungmin Park
  2012-11-20  7:03   ` Prathyush K
  0 siblings, 1 reply; 5+ messages in thread
From: Kyungmin Park @ 2012-11-19  9:44 UTC (permalink / raw)
  To: Prathyush K, 대인기; +Cc: dri-devel

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
  2012-11-19  9:44 ` Kyungmin Park
@ 2012-11-20  7:03   ` Prathyush K
  2012-11-20  7:19     ` Kyungmin Park
  0 siblings, 1 reply; 5+ messages in thread
From: Prathyush K @ 2012-11-20  7:03 UTC (permalink / raw)
  To: Kyungmin Park; +Cc: dri-devel, Prathyush K


[-- Attachment #1.1: Type: text/plain, Size: 8553 bytes --]

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
>

[-- Attachment #1.2: Type: text/html, Size: 11470 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
  2012-11-20  7:03   ` Prathyush K
@ 2012-11-20  7:19     ` Kyungmin Park
  2012-11-20  9:10       ` Prathyush K
  0 siblings, 1 reply; 5+ messages in thread
From: Kyungmin Park @ 2012-11-20  7:19 UTC (permalink / raw)
  To: Prathyush K; +Cc: dri-devel, Prathyush K

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer
  2012-11-20  7:19     ` Kyungmin Park
@ 2012-11-20  9:10       ` Prathyush K
  0 siblings, 0 replies; 5+ messages in thread
From: Prathyush K @ 2012-11-20  9:10 UTC (permalink / raw)
  To: Kyungmin Park; +Cc: dri-devel, Prathyush K


[-- Attachment #1.1: Type: text/plain, Size: 10487 bytes --]

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

[-- Attachment #1.2: Type: text/html, Size: 15520 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-11-20  9:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-15 10:25 [PATCH] drm/exynos: remove 'pages' and 'page_size' elements in exynos gem buffer Prathyush K
2012-11-19  9:44 ` Kyungmin Park
2012-11-20  7:03   ` Prathyush K
2012-11-20  7:19     ` Kyungmin Park
2012-11-20  9:10       ` Prathyush K

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.