diff mbox

[17/33] drm/omap: remove support for ext mem & sync

Message ID 1455875288-4370-18-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen Feb. 19, 2016, 9:47 a.m. UTC 
We no longer have the omapdrm plugin system for SGX, and we can thus
remove the support for external memory and sync objects from omap_gem.c.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 91 +++++++-------------------------------
 1 file changed, 16 insertions(+), 75 deletions(-)

Comments

Laurent Pinchart Feb. 23, 2016, 10:42 p.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:52 Tomi Valkeinen wrote:
> We no longer have the omapdrm plugin system for SGX, and we can thus
> remove the support for external memory and sync objects from omap_gem.c.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 91 ++++++-----------------------------
>  1 file changed, 16 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index db5dbdb8cd0a..52e4a6c95058
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -33,9 +33,7 @@
>  /* note: we use upper 8 bits of flags for driver-internal flags: */
>  #define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the
>  dma_alloc_* API */
>  #define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through
>  shmem backing */
> -#define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
>  #define OMAP_BO_MEM_DMABUF	0x08000000	/* memory imported from a dmabuf
> */
> -#define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object
> */
> 
>  struct omap_gem_object {
>  	struct drm_gem_object base;
> @@ -1177,20 +1175,6 @@ unlock:
>  	return ret;
>  }
> 
> -/* it is a bit lame to handle updates in this sort of polling way, but
> - * in case of PVR, the GPU can directly update read/write complete
> - * values, and not really tell us which ones it updated.. this also
> - * means that sync_lock is not quite sufficient.  So we'll need to
> - * do something a bit better when it comes time to add support for
> - * separate 2d hw..
> - */
> -void omap_gem_op_update(void)

The function is still referenced from a comment above omap_gem_op_async().

> -{
> -	spin_lock(&sync_lock);
> -	sync_op_update();
> -	spin_unlock(&sync_lock);
> -}
> -
>  /* mark the start of read and/or write operation */
>  int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op)
>  {
> @@ -1300,43 +1284,6 @@ int omap_gem_op_async(struct drm_gem_object *obj,
> enum omap_gem_op op, return 0;
>  }
> 
> -/* special API so PVR can update the buffer to use a sync-object allocated
> - * from it's sync-obj heap.  Only used for a newly allocated (from PVR's
> - * perspective) sync-object, so we overwrite the new syncobj w/ values
> - * from the already allocated syncobj (if there is one)
> - */
> -int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)

And this one from a comment in the omap_gem_object() structure.

The rest looks good to me, after updating the comments,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -{
> -	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> -	int ret = 0;
> -
> -	spin_lock(&sync_lock);
> -
> -	if ((omap_obj->flags & OMAP_BO_EXT_SYNC) && !syncobj) {
> -		/* clearing a previously set syncobj */
> -		syncobj = kmemdup(omap_obj->sync, sizeof(*omap_obj->sync),
> -				  GFP_ATOMIC);
> -		if (!syncobj) {
> -			ret = -ENOMEM;
> -			goto unlock;
> -		}
> -		omap_obj->flags &= ~OMAP_BO_EXT_SYNC;
> -		omap_obj->sync = syncobj;
> -	} else if (syncobj && !(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
> -		/* replacing an existing syncobj */
> -		if (omap_obj->sync) {
> -			memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
> -			kfree(omap_obj->sync);
> -		}
> -		omap_obj->flags |= OMAP_BO_EXT_SYNC;
> -		omap_obj->sync = syncobj;
> -	}
> -
> -unlock:
> -	spin_unlock(&sync_lock);
> -	return ret;
> -}
> -
>  /*
> ---------------------------------------------------------------------------
> -- * Constructor & Destructor
>   */
> @@ -1360,28 +1307,23 @@ void omap_gem_free_object(struct drm_gem_object
> *obj) */
>  	WARN_ON(omap_obj->paddr_cnt > 0);
> 
> -	/* don't free externally allocated backing memory */
> -	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
> -		if (omap_obj->pages) {
> -			if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
> -				kfree(omap_obj->pages);
> -			else
> -				omap_gem_detach_pages(obj);
> -		}
> +	if (omap_obj->pages) {
> +		if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
> +			kfree(omap_obj->pages);
> +		else
> +			omap_gem_detach_pages(obj);
> +	}
> 
> -		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
> -			dma_free_writecombine(dev->dev, obj->size,
> -					omap_obj->vaddr, omap_obj->paddr);
> -		} else if (omap_obj->vaddr) {
> -			vunmap(omap_obj->vaddr);
> -		} else if (obj->import_attach) {
> -			drm_prime_gem_destroy(obj, omap_obj->sgt);
> -		}
> +	if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
> +		dma_free_writecombine(dev->dev, obj->size,
> +				omap_obj->vaddr, omap_obj->paddr);
> +	} else if (omap_obj->vaddr) {
> +		vunmap(omap_obj->vaddr);
> +	} else if (obj->import_attach) {
> +		drm_prime_gem_destroy(obj, omap_obj->sgt);
>  	}
> 
> -	/* don't free externally allocated syncobj */
> -	if (!(omap_obj->flags & OMAP_BO_EXT_SYNC))
> -		kfree(omap_obj->sync);
> +	kfree(omap_obj->sync);
> 
>  	drm_gem_object_release(obj);
> 
> @@ -1426,10 +1368,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device
> *dev, * use contiguous memory only if no TILER is available.
>  		 */
>  		flags |= OMAP_BO_MEM_DMA_API;
> -	} else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) {
> +	} else if (!(flags & OMAP_BO_MEM_DMABUF)) {
>  		/*
> -		 * All other buffers not backed by external memory or dma_buf
> -		 * are shmem-backed.
> +		 * All other buffers not backed by dma_buf are shmem-backed.
>  		 */
>  		flags |= OMAP_BO_MEM_SHMEM;
>  	}
Tomi Valkeinen Feb. 24, 2016, 9:38 a.m. UTC | #2 
On 24/02/16 00:42, Laurent Pinchart wrote:

>> -void omap_gem_op_update(void)
> 
> The function is still referenced from a comment above omap_gem_op_async().

Right. I changed the comment to refer to omap_gem_op_finish().

>> -int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)
> 
> And this one from a comment in the omap_gem_object() structure.

Yep. I think almost all of the comment can be just removed, as SGX no
longer uses this:

@@ -105,17 +105,7 @@ struct omap_gem_object {
         * sync-object allocated on demand (if needed)
         *
         * Per-buffer sync-object for tracking pending and completed hw/dma
-        * read and write operations.  The layout in memory is dictated by
-        * the SGX firmware, which uses this information to stall the command
-        * stream if a surface is not ready yet.
-        *
-        * Note that when buffer is used by SGX, the sync-object needs to be
-        * allocated from a special heap of sync-objects.  This way many sync
-        * objects can be packed in a page, and not waste GPU virtual address
-        * space.  Because of this we have to have a omap_gem_set_sync_object()
-        * API to allow replacement of the syncobj after it has (potentially)
-        * already been allocated.  A bit ugly but I haven't thought of a
-        * better alternative.
+        * read and write operations.
         */
        struct {
                uint32_t write_pending;

 Tomi
diff mbox

Patch

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index db5dbdb8cd0a..52e4a6c95058 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -33,9 +33,7 @@ 
 /* note: we use upper 8 bits of flags for driver-internal flags: */
 #define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the dma_alloc_* API */
 #define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through shmem backing */
-#define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
 #define OMAP_BO_MEM_DMABUF	0x08000000	/* memory imported from a dmabuf */
-#define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object */
 
 struct omap_gem_object {
 	struct drm_gem_object base;
@@ -1177,20 +1175,6 @@  unlock:
 	return ret;
 }
 
-/* it is a bit lame to handle updates in this sort of polling way, but
- * in case of PVR, the GPU can directly update read/write complete
- * values, and not really tell us which ones it updated.. this also
- * means that sync_lock is not quite sufficient.  So we'll need to
- * do something a bit better when it comes time to add support for
- * separate 2d hw..
- */
-void omap_gem_op_update(void)
-{
-	spin_lock(&sync_lock);
-	sync_op_update();
-	spin_unlock(&sync_lock);
-}
-
 /* mark the start of read and/or write operation */
 int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op)
 {
@@ -1300,43 +1284,6 @@  int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op,
 	return 0;
 }
 
-/* special API so PVR can update the buffer to use a sync-object allocated
- * from it's sync-obj heap.  Only used for a newly allocated (from PVR's
- * perspective) sync-object, so we overwrite the new syncobj w/ values
- * from the already allocated syncobj (if there is one)
- */
-int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	int ret = 0;
-
-	spin_lock(&sync_lock);
-
-	if ((omap_obj->flags & OMAP_BO_EXT_SYNC) && !syncobj) {
-		/* clearing a previously set syncobj */
-		syncobj = kmemdup(omap_obj->sync, sizeof(*omap_obj->sync),
-				  GFP_ATOMIC);
-		if (!syncobj) {
-			ret = -ENOMEM;
-			goto unlock;
-		}
-		omap_obj->flags &= ~OMAP_BO_EXT_SYNC;
-		omap_obj->sync = syncobj;
-	} else if (syncobj && !(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
-		/* replacing an existing syncobj */
-		if (omap_obj->sync) {
-			memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
-			kfree(omap_obj->sync);
-		}
-		omap_obj->flags |= OMAP_BO_EXT_SYNC;
-		omap_obj->sync = syncobj;
-	}
-
-unlock:
-	spin_unlock(&sync_lock);
-	return ret;
-}
-
 /* -----------------------------------------------------------------------------
  * Constructor & Destructor
  */
@@ -1360,28 +1307,23 @@  void omap_gem_free_object(struct drm_gem_object *obj)
 	 */
 	WARN_ON(omap_obj->paddr_cnt > 0);
 
-	/* don't free externally allocated backing memory */
-	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
-		if (omap_obj->pages) {
-			if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
-				kfree(omap_obj->pages);
-			else
-				omap_gem_detach_pages(obj);
-		}
+	if (omap_obj->pages) {
+		if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
+			kfree(omap_obj->pages);
+		else
+			omap_gem_detach_pages(obj);
+	}
 
-		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
-			dma_free_writecombine(dev->dev, obj->size,
-					omap_obj->vaddr, omap_obj->paddr);
-		} else if (omap_obj->vaddr) {
-			vunmap(omap_obj->vaddr);
-		} else if (obj->import_attach) {
-			drm_prime_gem_destroy(obj, omap_obj->sgt);
-		}
+	if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
+		dma_free_writecombine(dev->dev, obj->size,
+				omap_obj->vaddr, omap_obj->paddr);
+	} else if (omap_obj->vaddr) {
+		vunmap(omap_obj->vaddr);
+	} else if (obj->import_attach) {
+		drm_prime_gem_destroy(obj, omap_obj->sgt);
 	}
 
-	/* don't free externally allocated syncobj */
-	if (!(omap_obj->flags & OMAP_BO_EXT_SYNC))
-		kfree(omap_obj->sync);
+	kfree(omap_obj->sync);
 
 	drm_gem_object_release(obj);
 
@@ -1426,10 +1368,9 @@  struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		 * use contiguous memory only if no TILER is available.
 		 */
 		flags |= OMAP_BO_MEM_DMA_API;
-	} else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) {
+	} else if (!(flags & OMAP_BO_MEM_DMABUF)) {
 		/*
-		 * All other buffers not backed by external memory or dma_buf
-		 * are shmem-backed.
+		 * All other buffers not backed by dma_buf are shmem-backed.
 		 */
 		flags |= OMAP_BO_MEM_SHMEM;
 	}