Re: [Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

2018-03-27 Thread Daniele Ceraolo Spurio 



On 27/03/18 14:05, Michal Wajdeczko wrote:
On Tue, 27 Mar 2018 22:03:23 +0200, Michel Thierry 
 wrote:



On 3/27/2018 11:25 AM, Daniele Ceraolo Spurio wrote:

  On 26/03/18 12:48, Michal Wajdeczko wrote:

When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko 
Cc: Oscar Mateo 
Reviewed-by: Michel Thierry 
---
  drivers/gpu/drm/i915/intel_guc.c    | 5 +
  drivers/gpu/drm/i915/intel_guc.h    | 1 +
  drivers/gpu/drm/i915/intel_guc_ct.c | 9 +
  3 files changed, 15 insertions(+)

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

index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void 
intel_guc_to_host_event_handler_mmio(struct intel_guc *guc)

  I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
  spin_unlock(>irq_lock);
+    intel_guc_to_host_process_recv_msg(guc, msg);
+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 
msg)

+{
  if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
  intel_guc_log_handle_flush_event(>log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h 
b/drivers/gpu/drm/i915/intel_guc.h

index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, 
const u32 *action, u32 len,

  void intel_guc_to_host_event_handler(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 
msg);

  int intel_guc_sample_forcewake(struct intel_guc *guc);
  int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
  int intel_guc_suspend(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c

index aa810ad..e837084 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -694,8 +694,17 @@ static int ct_handle_response(struct 
intel_guc_ct *ct, const u32 *msg)

  static void ct_process_request(struct intel_guc_ct *ct,
 u32 action, u32 len, const u32 *payload)
  {
+    struct intel_guc *guc = ct_to_guc(ct);
+
  switch (action) {
+    case INTEL_GUC_ACTION_DEFAULT:
+    if (unlikely(len < 1))
+    goto fail_unexpected;
 Don't we need to remove the non-enabled messages here? i.e. like we 
do for the mmio receive:

  msg = *payload & guc->msg_enabled_mask;
 otherwise I think we'll try to process log interrupts even if the 
relay is not enabled.


In Daniele's specific example, I still think 
guc_log_enable_flush_events should have been called (and don't do 
anything inside handle_flush_event, the goal was to not serve it).


But it's true that we should filter any unexpected incoming request (& 
guc->msg_enabled_mask), and any new user should be calling 
intel_guc_enable_msg during its setup.


hmm, my understanding was that purpose of msg_enabled_mask was to:

  * Sample the log buffer flush related bits & clear them out now
  * itself from the message identity register to minimize the
  * probability of losing a flush interrupt, when there are back
  * to back flush interrupts.

and is not applicable to messages sent over CTB, as we can't miss msg.

if you still believe that filtering is required, it should not be done
here, as purpose of this function is to verify message completeness and
call actual handler, but in intel_guc_to_host_process_recv_msg(), where
format of received data is known.

/m



The problem is that now we keep the interrupts enabled even if the log 
relay is not enabled. If we handle the interrupt anyway and queue the 
flush_work (from intel_guc_log_handle_flush_event), we end up hitting:


if (WARN_ON(!intel_guc_log_relay_enabled(log)))
goto out_unlock;

inside guc_read_update_log_buffer.

I don't think that the comment you added above referred to the specific 
usage of msg_enabled_mask but more to the fact that we clean the bits 
related to the messages we're going to handle.


Daniele



Thanks,

 Daniele


+    intel_guc_to_host_process_recv_msg(guc, *payload);
+    break;
+
  default:
+fail_unexpected:
  DRM_ERROR("CT: unexpected request %x %*phn\n",
    action, 4 * len, payload);
  break;

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

2018-03-27 Thread Michal Wajdeczko 
On Tue, 27 Mar 2018 22:03:23 +0200, Michel Thierry  
 wrote:



On 3/27/2018 11:25 AM, Daniele Ceraolo Spurio wrote:

  On 26/03/18 12:48, Michal Wajdeczko wrote:

When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko 
Cc: Oscar Mateo 
Reviewed-by: Michel Thierry 
---
  drivers/gpu/drm/i915/intel_guc.c| 5 +
  drivers/gpu/drm/i915/intel_guc.h| 1 +
  drivers/gpu/drm/i915/intel_guc_ct.c | 9 +
  3 files changed, 15 insertions(+)

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

index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct  
intel_guc *guc)

  I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
  spin_unlock(>irq_lock);
+intel_guc_to_host_process_recv_msg(guc, msg);
+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32  
msg)

+{
  if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
  intel_guc_log_handle_flush_event(>log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h  
b/drivers/gpu/drm/i915/intel_guc.h

index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc,  
const u32 *action, u32 len,

  void intel_guc_to_host_event_handler(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32  
msg);

  int intel_guc_sample_forcewake(struct intel_guc *guc);
  int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
  int intel_guc_suspend(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c  
b/drivers/gpu/drm/i915/intel_guc_ct.c

index aa810ad..e837084 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -694,8 +694,17 @@ static int ct_handle_response(struct intel_guc_ct  
*ct, const u32 *msg)

  static void ct_process_request(struct intel_guc_ct *ct,
 u32 action, u32 len, const u32 *payload)
  {
+struct intel_guc *guc = ct_to_guc(ct);
+
  switch (action) {
+case INTEL_GUC_ACTION_DEFAULT:
+if (unlikely(len < 1))
+goto fail_unexpected;
 Don't we need to remove the non-enabled messages here? i.e. like we do  
for the mmio receive:

  msg = *payload & guc->msg_enabled_mask;
 otherwise I think we'll try to process log interrupts even if the  
relay is not enabled.


In Daniele's specific example, I still think guc_log_enable_flush_events  
should have been called (and don't do anything inside  
handle_flush_event, the goal was to not serve it).


But it's true that we should filter any unexpected incoming request (&  
guc->msg_enabled_mask), and any new user should be calling  
intel_guc_enable_msg during its setup.


hmm, my understanding was that purpose of msg_enabled_mask was to:

 * Sample the log buffer flush related bits & clear them out now
 * itself from the message identity register to minimize the
 * probability of losing a flush interrupt, when there are back
 * to back flush interrupts.

and is not applicable to messages sent over CTB, as we can't miss msg.

if you still believe that filtering is required, it should not be done
here, as purpose of this function is to verify message completeness and
call actual handler, but in intel_guc_to_host_process_recv_msg(), where
format of received data is known.

/m



Thanks,

 Daniele


+intel_guc_to_host_process_recv_msg(guc, *payload);
+break;
+
  default:
+fail_unexpected:
  DRM_ERROR("CT: unexpected request %x %*phn\n",
action, 4 * len, payload);
  break;

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

2018-03-27 Thread Michel Thierry 

On 3/27/2018 11:25 AM, Daniele Ceraolo Spurio wrote:



On 26/03/18 12:48, Michal Wajdeczko wrote:

When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko 
Cc: Oscar Mateo 
Reviewed-by: Michel Thierry 
---
  drivers/gpu/drm/i915/intel_guc.c    | 5 +
  drivers/gpu/drm/i915/intel_guc.h    | 1 +
  drivers/gpu/drm/i915/intel_guc_ct.c | 9 +
  3 files changed, 15 insertions(+)

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

index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct 
intel_guc *guc)

  I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
  spin_unlock(>irq_lock);
+    intel_guc_to_host_process_recv_msg(guc, msg);
+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg)
+{
  if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
 INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
  intel_guc_log_handle_flush_event(>log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h 
b/drivers/gpu/drm/i915/intel_guc.h

index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, 
const u32 *action, u32 len,

  void intel_guc_to_host_event_handler(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg);
  int intel_guc_sample_forcewake(struct intel_guc *guc);
  int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
  int intel_guc_suspend(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c

index aa810ad..e837084 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -694,8 +694,17 @@ static int ct_handle_response(struct intel_guc_ct 
*ct, const u32 *msg)

  static void ct_process_request(struct intel_guc_ct *ct,
 u32 action, u32 len, const u32 *payload)
  {
+    struct intel_guc *guc = ct_to_guc(ct);
+
  switch (action) {
+    case INTEL_GUC_ACTION_DEFAULT:
+    if (unlikely(len < 1))
+    goto fail_unexpected;


Don't we need to remove the non-enabled messages here? i.e. like we do 
for the mmio receive:


 msg = *payload & guc->msg_enabled_mask;

otherwise I think we'll try to process log interrupts even if the relay 
is not enabled.


In Daniele's specific example, I still think guc_log_enable_flush_events 
should have been called (and don't do anything inside 
handle_flush_event, the goal was to not serve it).


But it's true that we should filter any unexpected incoming request (& 
guc->msg_enabled_mask), and any new user should be calling 
intel_guc_enable_msg during its setup.


Thanks,


Daniele


+    intel_guc_to_host_process_recv_msg(guc, *payload);
+    break;
+
  default:
+fail_unexpected:
  DRM_ERROR("CT: unexpected request %x %*phn\n",
    action, 4 * len, payload);
  break;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

2018-03-27 Thread Daniele Ceraolo Spurio 



On 26/03/18 12:48, Michal Wajdeczko wrote:

When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko 
Cc: Oscar Mateo 
Reviewed-by: Michel Thierry 
---
  drivers/gpu/drm/i915/intel_guc.c| 5 +
  drivers/gpu/drm/i915/intel_guc.h| 1 +
  drivers/gpu/drm/i915/intel_guc_ct.c | 9 +
  3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct intel_guc 
*guc)
I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
spin_unlock(>irq_lock);
  
+	intel_guc_to_host_process_recv_msg(guc, msg);

+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg)
+{
if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
intel_guc_log_handle_flush_event(>log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 
*action, u32 len,
  void intel_guc_to_host_event_handler(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
  void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg);
  int intel_guc_sample_forcewake(struct intel_guc *guc);
  int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
  int intel_guc_suspend(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c
index aa810ad..e837084 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -694,8 +694,17 @@ static int ct_handle_response(struct intel_guc_ct *ct, 
const u32 *msg)
  static void ct_process_request(struct intel_guc_ct *ct,
   u32 action, u32 len, const u32 *payload)
  {
+   struct intel_guc *guc = ct_to_guc(ct);
+
switch (action) {
+   case INTEL_GUC_ACTION_DEFAULT:
+   if (unlikely(len < 1))
+   goto fail_unexpected;


Don't we need to remove the non-enabled messages here? i.e. like we do 
for the mmio receive:


msg = *payload & guc->msg_enabled_mask;

otherwise I think we'll try to process log interrupts even if the relay 
is not enabled.


Daniele


+   intel_guc_to_host_process_recv_msg(guc, *payload);
+   break;
+
default:
+fail_unexpected:
DRM_ERROR("CT: unexpected request %x %*phn\n",
  action, 4 * len, payload);
break;


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 10/12] drm/i915/guc: Handle default action received over CT

2018-03-26 Thread Michal Wajdeczko 
When running on platform with CTB based GuC communication enabled,
GuC to Host event data will be delivered as CT request message.
However, content of the data[1] of this CT message follows format
of the scratch register used in MMIO based communication, so some
code reuse is still possible.

Signed-off-by: Michal Wajdeczko 
Cc: Oscar Mateo 
Reviewed-by: Michel Thierry 
---
 drivers/gpu/drm/i915/intel_guc.c| 5 +
 drivers/gpu/drm/i915/intel_guc.h| 1 +
 drivers/gpu/drm/i915/intel_guc_ct.c | 9 +
 3 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 118db81..b6d2778 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -416,6 +416,11 @@ void intel_guc_to_host_event_handler_mmio(struct intel_guc 
*guc)
I915_WRITE(SOFT_SCRATCH(15), val & ~msg);
spin_unlock(>irq_lock);
 
+   intel_guc_to_host_process_recv_msg(guc, msg);
+}
+
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg)
+{
if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |
   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED))
intel_guc_log_handle_flush_event(>log);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 6dc109a..f1265e1 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -163,6 +163,7 @@ int intel_guc_send_mmio(struct intel_guc *guc, const u32 
*action, u32 len,
 void intel_guc_to_host_event_handler(struct intel_guc *guc);
 void intel_guc_to_host_event_handler_nop(struct intel_guc *guc);
 void intel_guc_to_host_event_handler_mmio(struct intel_guc *guc);
+void intel_guc_to_host_process_recv_msg(struct intel_guc *guc, u32 msg);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
 int intel_guc_suspend(struct intel_guc *guc);
diff --git a/drivers/gpu/drm/i915/intel_guc_ct.c 
b/drivers/gpu/drm/i915/intel_guc_ct.c
index aa810ad..e837084 100644
--- a/drivers/gpu/drm/i915/intel_guc_ct.c
+++ b/drivers/gpu/drm/i915/intel_guc_ct.c
@@ -694,8 +694,17 @@ static int ct_handle_response(struct intel_guc_ct *ct, 
const u32 *msg)
 static void ct_process_request(struct intel_guc_ct *ct,
   u32 action, u32 len, const u32 *payload)
 {
+   struct intel_guc *guc = ct_to_guc(ct);
+
switch (action) {
+   case INTEL_GUC_ACTION_DEFAULT:
+   if (unlikely(len < 1))
+   goto fail_unexpected;
+   intel_guc_to_host_process_recv_msg(guc, *payload);
+   break;
+
default:
+fail_unexpected:
DRM_ERROR("CT: unexpected request %x %*phn\n",
  action, 4 * len, payload);
break;
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx