Re: [Intel-gfx] [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

2017-02-16 Thread Chris Wilson 
On Thu, Feb 16, 2017 at 06:15:05AM -0800, Oscar Mateo wrote:
>  static void guc_ctx_desc_init(struct intel_guc *guc,
> struct i915_guc_client *client)
>  {
>   struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   struct intel_engine_cs *engine;
>   struct i915_gem_context *ctx = client->owner;
> - struct guc_context_desc desc;
> - struct sg_table *sg;
> + struct guc_context_desc *desc;
>   unsigned int tmp;
>   u32 gfx_addr;
>  
> - memset(, 0, sizeof(desc));
> + desc = __get_context_desc(client);

Do you want to make the assumption that these are zeroed-on-create
objects? We could switch to using non-swappable (internal) objects that
are not cleared on create. i.e.

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 4a752f2b6e24..4128e8937b45 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -650,7 +650,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc 
*guc, u32 size)
struct i915_vma *vma;
int ret;
 
-   obj = i915_gem_object_create(dev_priv, size);
+   obj = i915_gem_object_create_internal(dev_priv, size);
if (IS_ERR(obj))
return ERR_CAST(obj);
 
Or do we write the entire desc? It doesn't look like we do, but do we do
enough?

Other than that potential booby trap for later,
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

2017-02-16 Thread Oscar Mateo 

Onion teardown in a separate patch (since it addresses a separate problem).


On 02/16/2017 06:15 AM, Oscar Mateo wrote:

The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.

Also, Chris abhors scatterlists :)

v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/

Signed-off-by: Oscar Mateo 
---
  drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++
  drivers/gpu/drm/i915/intel_guc_loader.c|  2 +-
  drivers/gpu/drm/i915/intel_uc.h|  3 +-
  3 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3ea2c71..f7d9897 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc 
*guc, u32 ctx_index)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
  }
  
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)

+{
+   return client->guc->ctx_pool_vaddr +
+   sizeof(struct guc_context_desc) * client->ctx_index;
+}
+
  /*
   * Initialise, update, or clear doorbell data shared with the GuC
   *
@@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc 
*guc, u32 ctx_index)
  
  static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)

  {
-   struct sg_table *sg = client->guc->ctx_pool_vma->pages;
-   struct guc_context_desc desc;
-   size_t len;
+   struct guc_context_desc *desc;
  
  	/* Update the GuC's idea of the doorbell ID */

-   len = sg_pcopy_to_buffer(sg->sgl, sg->nents, , sizeof(desc),
-sizeof(desc) * client->ctx_index);
-   if (len != sizeof(desc))
-   return -EFAULT;
-
-   desc.db_id = new_id;
-   len = sg_pcopy_from_buffer(sg->sgl, sg->nents, , sizeof(desc),
-  sizeof(desc) * client->ctx_index);
-   if (len != sizeof(desc))
-   return -EFAULT;
+   desc = __get_context_desc(client);
+   desc->db_id = new_id;
  
  	return 0;

  }
@@ -270,29 +266,27 @@ static void guc_proc_desc_init(struct intel_guc *guc,
   * data structures relating to this client (doorbell, process descriptor,
   * write queue, etc).
   */
-
  static void guc_ctx_desc_init(struct intel_guc *guc,
  struct i915_guc_client *client)
  {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
-   struct guc_context_desc desc;
-   struct sg_table *sg;
+   struct guc_context_desc *desc;
unsigned int tmp;
u32 gfx_addr;
  
-	memset(, 0, sizeof(desc));

+   desc = __get_context_desc(client);
  
-	desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;

-   desc.context_id = client->ctx_index;
-   desc.priority = client->priority;
-   desc.db_id = client->doorbell_id;
+   desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+   desc->context_id = client->ctx_index;
+   desc->priority = client->priority;
+   desc->db_id = client->doorbell_id;
  
  	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {

struct intel_context *ce = >engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
-   struct guc_execlist_context *lrc = [guc_engine_id];
+   struct guc_execlist_context *lrc = >lrc[guc_engine_id];
  
  		/* TODO: We have a design issue to be solved here. Only when we

 * receive the first batch, we know which engine is used by the
@@ -317,50 +311,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
  
-		desc.engines_used |= (1 << guc_engine_id);

+   desc->engines_used |= (1 << guc_engine_id);
}
  
  	DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",

-   client->engines, desc.engines_used);
-   WARN_ON(desc.engines_used == 0);
+   client->engines, desc->engines_used);
+   WARN_ON(desc->engines_used == 0);
  
  	/*

 * The doorbell, process descriptor, and workqueue are all parts
 * of the client object, which the GuC will reference via the GGTT
 */
gfx_addr = guc_ggtt_offset(client->vma);
-   desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+   desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+   client->doorbell_offset;
+   desc->db_trigger_cpu = (uintptr_t)client->vaddr +

[Intel-gfx] [PATCH v2] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

2017-02-16 Thread Oscar Mateo 
The GuC descriptor is big in size. If we use a local definition of
guc_desc we have a chance to overflow stack, so avoid it.

Also, Chris abhors scatterlists :)

v2: Rebased, helper function to retrieve the context descriptor,
s/ctx_pool_vma/ctx_pool/

Signed-off-by: Oscar Mateo 
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 94 ++
 drivers/gpu/drm/i915/intel_guc_loader.c|  2 +-
 drivers/gpu/drm/i915/intel_uc.h|  3 +-
 3 files changed, 48 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c 
b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3ea2c71..f7d9897 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -134,6 +134,12 @@ static int __guc_deallocate_doorbell(struct intel_guc 
*guc, u32 ctx_index)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
+static struct guc_context_desc *__get_context_desc(struct i915_guc_client 
*client)
+{
+   return client->guc->ctx_pool_vaddr +
+   sizeof(struct guc_context_desc) * client->ctx_index;
+}
+
 /*
  * Initialise, update, or clear doorbell data shared with the GuC
  *
@@ -143,21 +149,11 @@ static int __guc_deallocate_doorbell(struct intel_guc 
*guc, u32 ctx_index)
 
 static int __update_doorbell_desc(struct i915_guc_client *client, u16 new_id)
 {
-   struct sg_table *sg = client->guc->ctx_pool_vma->pages;
-   struct guc_context_desc desc;
-   size_t len;
+   struct guc_context_desc *desc;
 
/* Update the GuC's idea of the doorbell ID */
-   len = sg_pcopy_to_buffer(sg->sgl, sg->nents, , sizeof(desc),
-sizeof(desc) * client->ctx_index);
-   if (len != sizeof(desc))
-   return -EFAULT;
-
-   desc.db_id = new_id;
-   len = sg_pcopy_from_buffer(sg->sgl, sg->nents, , sizeof(desc),
-  sizeof(desc) * client->ctx_index);
-   if (len != sizeof(desc))
-   return -EFAULT;
+   desc = __get_context_desc(client);
+   desc->db_id = new_id;
 
return 0;
 }
@@ -270,29 +266,27 @@ static void guc_proc_desc_init(struct intel_guc *guc,
  * data structures relating to this client (doorbell, process descriptor,
  * write queue, etc).
  */
-
 static void guc_ctx_desc_init(struct intel_guc *guc,
  struct i915_guc_client *client)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct intel_engine_cs *engine;
struct i915_gem_context *ctx = client->owner;
-   struct guc_context_desc desc;
-   struct sg_table *sg;
+   struct guc_context_desc *desc;
unsigned int tmp;
u32 gfx_addr;
 
-   memset(, 0, sizeof(desc));
+   desc = __get_context_desc(client);
 
-   desc.attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
-   desc.context_id = client->ctx_index;
-   desc.priority = client->priority;
-   desc.db_id = client->doorbell_id;
+   desc->attribute = GUC_CTX_DESC_ATTR_ACTIVE | GUC_CTX_DESC_ATTR_KERNEL;
+   desc->context_id = client->ctx_index;
+   desc->priority = client->priority;
+   desc->db_id = client->doorbell_id;
 
for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
struct intel_context *ce = >engine[engine->id];
uint32_t guc_engine_id = engine->guc_id;
-   struct guc_execlist_context *lrc = [guc_engine_id];
+   struct guc_execlist_context *lrc = >lrc[guc_engine_id];
 
/* TODO: We have a design issue to be solved here. Only when we
 * receive the first batch, we know which engine is used by the
@@ -317,50 +311,41 @@ static void guc_ctx_desc_init(struct intel_guc *guc,
lrc->ring_next_free_location = lrc->ring_begin;
lrc->ring_current_tail_pointer_value = 0;
 
-   desc.engines_used |= (1 << guc_engine_id);
+   desc->engines_used |= (1 << guc_engine_id);
}
 
DRM_DEBUG_DRIVER("Host engines 0x%x => GuC engines used 0x%x\n",
-   client->engines, desc.engines_used);
-   WARN_ON(desc.engines_used == 0);
+   client->engines, desc->engines_used);
+   WARN_ON(desc->engines_used == 0);
 
/*
 * The doorbell, process descriptor, and workqueue are all parts
 * of the client object, which the GuC will reference via the GGTT
 */
gfx_addr = guc_ggtt_offset(client->vma);
-   desc.db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+   desc->db_trigger_phy = sg_dma_address(client->vma->pages->sgl) +
+   client->doorbell_offset;
+   desc->db_trigger_cpu = (uintptr_t)client->vaddr +
client->doorbell_offset;
-   desc.db_trigger_cpu =
-   (uintptr_t)client->vaddr +