subject:"\[Intel\-gfx\] \[PATCH 1\/2\] drm\/i915\/guc\: Splitting CT channel open\/close functions"

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

2019-02-19 Thread Sujaritha



On 2/14/19 2:46 PM, Daniele Ceraolo Spurio wrote:



On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:

The aim of this patch is to allow enabling and disabling
of CTB without requiring the mutex lock.

Cc: Daniele Ceraolo Spurio 
Cc: Michal Wajdeczko 
Signed-off-by: Sujaritha Sundaresan 
---
  drivers/gpu/drm/i915/intel_guc.c    | 12 
  drivers/gpu/drm/i915/intel_guc_ct.c | 85 +
  drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
  3 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c 
b/drivers/gpu/drm/i915/intel_guc.c

index 8660af3fd755..8ecb47087457 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
  goto err_log;
  GEM_BUG_ON(!guc->ads_vma);
  +    if (HAS_GUC_CT(dev_priv)) {
+    ret = intel_guc_ct_init(&guc->ct);
+    if (ret)
+    goto err_ads;
+    }
+
  /* We need to notify the guc whenever we change the GGTT */
  i915_ggtt_enable_guc(dev_priv);
    return 0;
  +err_ads:
+    intel_guc_ads_destroy(guc);
  err_log:
  intel_guc_log_destroy(&guc->log);
  err_shared:
@@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
  struct drm_i915_private *dev_priv = guc_to_i915(guc);
    i915_ggtt_disable_guc(dev_priv);
+
+    if (HAS_GUC_CT(dev_priv))
+    intel_guc_ct_fini(&guc->ct);
+
  intel_guc_ads_destroy(guc);
  intel_guc_log_destroy(&guc->log);
  guc_shared_data_destroy(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c

index a52883e9146f..fbf9da247975 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct 
intel_guc *guc,

  return err;
  }
  -static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
+static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
  {
-    return ctch->vma != NULL;
+    return ctch->is_enabled;


Bikeshed: now that the check is simpler, doing ctch->enabled is 
actually simpler then ctch_is_enabled(ctch), so I'd prefer to just 
switch.


Will make this change.




  }
    static int ctch_init(struct intel_guc *guc,
@@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
  i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
  }
  -static int ctch_open(struct intel_guc *guc,
+static int ctch_enable(struct intel_guc *guc,
   struct intel_guc_ct_channel *ctch)
  {
  u32 base;
  int err;
  int i;
  -    CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
-    ctch->owner, yesno(ctch_is_open(ctch)));
-
-    if (!ctch->vma) {
-    err = ctch_init(guc, ctch);
-    if (unlikely(err))
-    goto err_out;
-    GEM_BUG_ON(!ctch->vma);
-    }
+    GEM_BUG_ON(!ctch->vma);


GEM_BUG_ON(ctch->enabled) as well?



Why would this change ?




    /* vma should be already allocated and map'ed */
  base = intel_guc_ggtt_offset(guc, ctch->vma);
@@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
  base + PAGE_SIZE/4 * CTB_RECV,
  INTEL_GUC_CT_BUFFER_TYPE_RECV);
  if (unlikely(err))
-    goto err_fini;
+    goto err_out;
    err = guc_action_register_ct_buffer(guc,
  base + PAGE_SIZE/4 * CTB_SEND,
@@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
  if (unlikely(err))
  goto err_deregister;
  +    ctch->is_enabled = true;
+
  return 0;
    err_deregister:
  guc_action_deregister_ct_buffer(guc,
  ctch->owner,
  INTEL_GUC_CT_BUFFER_TYPE_RECV);
-err_fini:
-    ctch_fini(guc, ctch);
  err_out:
  DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, 
err);

  return err;
  }
  -static void ctch_close(struct intel_guc *guc,
+static void ctch_disable(struct intel_guc *guc,
 struct intel_guc_ct_channel *ctch)
  {
-    GEM_BUG_ON(!ctch_is_open(ctch));
+    GEM_BUG_ON(!ctch_is_enabled(ctch));
+
+    ctch->is_enabled = false;
    guc_action_deregister_ct_buffer(guc,
  ctch->owner,
@@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
  guc_action_deregister_ct_buffer(guc,
  ctch->owner,
  INTEL_GUC_CT_BUFFER_TYPE_RECV);
-    ctch_fini(guc, ctch);
  }
    static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
@@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
  u32 fence;
  int err;
  -    GEM_BUG_ON(!ctch_is_open(ctch));
+    GEM_BUG_ON(!ctch_is_enabled(ctch));
  GEM_BUG_ON(!len);
  GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
  GEM_BUG_ON(!response_buf && response_buf_size);
@@ -817,7 +810,7 @@ static void ct_process_host_channel(struct 
intel_guc_ct *ct)
  u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one extra

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

2019-02-15 Thread Daniele Ceraolo Spurio




On 2/14/19 1:45 PM, Sujaritha Sundaresan wrote:

The aim of this patch is to allow enabling and disabling
of CTB without requiring the mutex lock.

Cc: Daniele Ceraolo Spurio 
Cc: Michal Wajdeczko 
Signed-off-by: Sujaritha Sundaresan 
---
  drivers/gpu/drm/i915/intel_guc.c| 12 
  drivers/gpu/drm/i915/intel_guc_ct.c | 85 +
  drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
  3 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8660af3fd755..8ecb47087457 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
goto err_log;
GEM_BUG_ON(!guc->ads_vma);
  
+	if (HAS_GUC_CT(dev_priv)) {

+   ret = intel_guc_ct_init(&guc->ct);
+   if (ret)
+   goto err_ads;
+   }
+
/* We need to notify the guc whenever we change the GGTT */
i915_ggtt_enable_guc(dev_priv);
  
  	return 0;
  
+err_ads:

+   intel_guc_ads_destroy(guc);
  err_log:
intel_guc_log_destroy(&guc->log);
  err_shared:
@@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
struct drm_i915_private *dev_priv = guc_to_i915(guc);
  
  	i915_ggtt_disable_guc(dev_priv);

+
+   if (HAS_GUC_CT(dev_priv))
+   intel_guc_ct_fini(&guc->ct);
+
intel_guc_ads_destroy(guc);
intel_guc_log_destroy(&guc->log);
guc_shared_data_destroy(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c
index a52883e9146f..fbf9da247975 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct intel_guc 
*guc,
return err;
  }
  
-static bool ctch_is_open(struct intel_guc_ct_channel *ctch)

+static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
  {
-   return ctch->vma != NULL;
+   return ctch->is_enabled;


Bikeshed: now that the check is simpler, doing ctch->enabled is actually 
simpler then ctch_is_enabled(ctch), so I'd prefer to just switch.



  }
  
  static int ctch_init(struct intel_guc *guc,

@@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
  }
  
-static int ctch_open(struct intel_guc *guc,

+static int ctch_enable(struct intel_guc *guc,
 struct intel_guc_ct_channel *ctch)
  {
u32 base;
int err;
int i;
  
-	CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",

-   ctch->owner, yesno(ctch_is_open(ctch)));
-
-   if (!ctch->vma) {
-   err = ctch_init(guc, ctch);
-   if (unlikely(err))
-   goto err_out;
-   GEM_BUG_ON(!ctch->vma);
-   }
+   GEM_BUG_ON(!ctch->vma);


GEM_BUG_ON(ctch->enabled) as well?

  
  	/* vma should be already allocated and map'ed */

base = intel_guc_ggtt_offset(guc, ctch->vma);
@@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
base + PAGE_SIZE/4 * CTB_RECV,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
if (unlikely(err))
-   goto err_fini;
+   goto err_out;
  
  	err = guc_action_register_ct_buffer(guc,

base + PAGE_SIZE/4 * CTB_SEND,
@@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
if (unlikely(err))
goto err_deregister;
  
+	ctch->is_enabled = true;

+
return 0;
  
  err_deregister:

guc_action_deregister_ct_buffer(guc,
ctch->owner,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
-err_fini:
-   ctch_fini(guc, ctch);
  err_out:
DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
return err;
  }
  
-static void ctch_close(struct intel_guc *guc,

+static void ctch_disable(struct intel_guc *guc,
   struct intel_guc_ct_channel *ctch)
  {
-   GEM_BUG_ON(!ctch_is_open(ctch));
+   GEM_BUG_ON(!ctch_is_enabled(ctch));
+
+   ctch->is_enabled = false;
  
  	guc_action_deregister_ct_buffer(guc,

ctch->owner,
@@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
guc_action_deregister_ct_buffer(guc,
ctch->owner,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
-   ctch_fini(guc, ctch);
  }
  
  static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)

@@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
u32 fence;
int err;
  
-	GEM_BUG_ON(!ctch_is_open(ctch));

+   GEM_BUG_ON(!ctch_is_enabled(ctch));
GEM_BUG_ON(!len)

[Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

2019-02-14 Thread Sujaritha Sundaresan

The aim of this patch is to allow enabling and disabling
of CTB without requiring the mutex lock.

Cc: Daniele Ceraolo Spurio 
Cc: Michal Wajdeczko 
Signed-off-by: Sujaritha Sundaresan 
---
 drivers/gpu/drm/i915/intel_guc.c| 12 
 drivers/gpu/drm/i915/intel_guc_ct.c | 85 +
 drivers/gpu/drm/i915/intel_guc_ct.h |  3 +
 3 files changed, 77 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 8660af3fd755..8ecb47087457 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -203,11 +203,19 @@ int intel_guc_init(struct intel_guc *guc)
goto err_log;
GEM_BUG_ON(!guc->ads_vma);
 
+   if (HAS_GUC_CT(dev_priv)) {
+   ret = intel_guc_ct_init(&guc->ct);
+   if (ret)
+   goto err_ads;
+   }
+
/* We need to notify the guc whenever we change the GGTT */
i915_ggtt_enable_guc(dev_priv);
 
return 0;
 
+err_ads:
+   intel_guc_ads_destroy(guc);
 err_log:
intel_guc_log_destroy(&guc->log);
 err_shared:
@@ -222,6 +230,10 @@ void intel_guc_fini(struct intel_guc *guc)
struct drm_i915_private *dev_priv = guc_to_i915(guc);
 
i915_ggtt_disable_guc(dev_priv);
+
+   if (HAS_GUC_CT(dev_priv))
+   intel_guc_ct_fini(&guc->ct);
+
intel_guc_ads_destroy(guc);
intel_guc_log_destroy(&guc->log);
guc_shared_data_destroy(guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c
index a52883e9146f..fbf9da247975 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -140,9 +140,9 @@ static int guc_action_deregister_ct_buffer(struct intel_guc 
*guc,
return err;
 }
 
-static bool ctch_is_open(struct intel_guc_ct_channel *ctch)
+static bool ctch_is_enabled(struct intel_guc_ct_channel *ctch)
 {
-   return ctch->vma != NULL;
+   return ctch->is_enabled;
 }
 
 static int ctch_init(struct intel_guc *guc,
@@ -217,22 +217,14 @@ static void ctch_fini(struct intel_guc *guc,
i915_vma_unpin_and_release(&ctch->vma, I915_VMA_RELEASE_MAP);
 }
 
-static int ctch_open(struct intel_guc *guc,
+static int ctch_enable(struct intel_guc *guc,
 struct intel_guc_ct_channel *ctch)
 {
u32 base;
int err;
int i;
 
-   CT_DEBUG_DRIVER("CT: channel %d reopen=%s\n",
-   ctch->owner, yesno(ctch_is_open(ctch)));
-
-   if (!ctch->vma) {
-   err = ctch_init(guc, ctch);
-   if (unlikely(err))
-   goto err_out;
-   GEM_BUG_ON(!ctch->vma);
-   }
+   GEM_BUG_ON(!ctch->vma);
 
/* vma should be already allocated and map'ed */
base = intel_guc_ggtt_offset(guc, ctch->vma);
@@ -255,7 +247,7 @@ static int ctch_open(struct intel_guc *guc,
base + PAGE_SIZE/4 * CTB_RECV,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
if (unlikely(err))
-   goto err_fini;
+   goto err_out;
 
err = guc_action_register_ct_buffer(guc,
base + PAGE_SIZE/4 * CTB_SEND,
@@ -263,23 +255,25 @@ static int ctch_open(struct intel_guc *guc,
if (unlikely(err))
goto err_deregister;
 
+   ctch->is_enabled = true;
+
return 0;
 
 err_deregister:
guc_action_deregister_ct_buffer(guc,
ctch->owner,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
-err_fini:
-   ctch_fini(guc, ctch);
 err_out:
DRM_ERROR("CT: can't open channel %d; err=%d\n", ctch->owner, err);
return err;
 }
 
-static void ctch_close(struct intel_guc *guc,
+static void ctch_disable(struct intel_guc *guc,
   struct intel_guc_ct_channel *ctch)
 {
-   GEM_BUG_ON(!ctch_is_open(ctch));
+   GEM_BUG_ON(!ctch_is_enabled(ctch));
+
+   ctch->is_enabled = false;
 
guc_action_deregister_ct_buffer(guc,
ctch->owner,
@@ -287,7 +281,6 @@ static void ctch_close(struct intel_guc *guc,
guc_action_deregister_ct_buffer(guc,
ctch->owner,
INTEL_GUC_CT_BUFFER_TYPE_RECV);
-   ctch_fini(guc, ctch);
 }
 
 static u32 ctch_get_next_fence(struct intel_guc_ct_channel *ctch)
@@ -481,7 +474,7 @@ static int ctch_send(struct intel_guc_ct *ct,
u32 fence;
int err;
 
-   GEM_BUG_ON(!ctch_is_open(ctch));
+   GEM_BUG_ON(!ctch_is_enabled(ctch));
GEM_BUG_ON(!len);
GEM_BUG_ON(len & ~GUC_CT_MSG_LEN_MASK);
GEM_BUG_ON(!response_buf && response_buf_size);
@@ -817,7 +810,7 @@ static void ct_process_host_channel(struct intel_guc_ct *ct)
u32 msg[GUC_CT_MSG_LEN_MASK + 1]; /* one

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

[Intel-gfx] [PATCH 1/2] drm/i915/guc: Splitting CT channel open/close functions

3 matches

Site Navigation

Mail list logo

Footer information