Re: [Intel-gfx] [PATCH 10/15] drm/i915/guc: Get rid of GuC log runtime

[Chris Wilson]

Quoting Michał Winiarski (2018-02-27 12:52:25)
> Keeping a separate runtime struct is only making the code dealing with
> relay less clear to read. Let's get rid of it, keeping everything in the
> log instead.

Or is just misnamed s/runtime/relay/, shorten a few member names for
precision and take advantage of locals to make the code readable?

I think having the substruct is reasonable for the purpose of
documenting the association between members and importantly denoting the
lock.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 10/15] drm/i915/guc: Get rid of GuC log runtime

[Sagar Arun Kamble]

I think keeping the distinction between base/mandatory members and 
runtime/optional members is okay as they are handled differently.

What do others think?

another related query, should we alloc/dealloc guc_log work queue in 
relay_open/release?


On 2/27/2018 6:22 PM, Michał Winiarski wrote:

Keeping a separate runtime struct is only making the code dealing with
relay less clear to read. Let's get rid of it, keeping everything in the
log instead.

Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Michal Wajdeczko 
---
  drivers/gpu/drm/i915/intel_guc.c | 14 
  drivers/gpu/drm/i915/intel_guc_log.c | 68 ++--
  drivers/gpu/drm/i915/intel_guc_log.h | 13 +++
  3 files changed, 46 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 41f2c3b3c482..e3b6ae158a12 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -87,9 +87,9 @@ int intel_guc_init_wq(struct intel_guc *guc)
 * or scheduled later on resume. This way the handling of work
 * item can be kept same between system suspend & rpm suspend.
 */
-   guc->log.runtime.flush_wq = alloc_ordered_workqueue("i915-guc_log",
-   WQ_HIGHPRI | WQ_FREEZABLE);
-   if (!guc->log.runtime.flush_wq) {
+   guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+   WQ_HIGHPRI | WQ_FREEZABLE);
+   if (!guc->log.flush_wq) {
DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
return -ENOMEM;
}
@@ -112,7 +112,7 @@ int intel_guc_init_wq(struct intel_guc *guc)
guc->preempt_wq = alloc_ordered_workqueue("i915-guc_preempt",
  WQ_HIGHPRI);
if (!guc->preempt_wq) {
-   destroy_workqueue(guc->log.runtime.flush_wq);
+   destroy_workqueue(guc->log.flush_wq);
DRM_ERROR("Couldn't allocate workqueue for GuC "
  "preemption\n");
return -ENOMEM;
@@ -130,7 +130,7 @@ void intel_guc_fini_wq(struct intel_guc *guc)
USES_GUC_SUBMISSION(dev_priv))
destroy_workqueue(guc->preempt_wq);
  
-	destroy_workqueue(guc->log.runtime.flush_wq);

+   destroy_workqueue(guc->log.flush_wq);
  }
  
  static int guc_shared_data_create(struct intel_guc *guc)

@@ -389,8 +389,8 @@ void intel_guc_notification_handler(struct intel_guc *guc)
  
  	if (msg & (INTEL_GUC_RECV_MSG_FLUSH_LOG_BUFFER |

   INTEL_GUC_RECV_MSG_CRASH_DUMP_POSTED)) {
-   queue_work(guc->log.runtime.flush_wq,
-  &guc->log.runtime.flush_work);
+   queue_work(guc->log.flush_wq,
+  &guc->log.flush_work);
  
  		guc->log.flush_interrupt_count++;

}
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
b/drivers/gpu/drm/i915/intel_guc_log.c
index d2aca10ab986..f95e18be1c5f 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -156,10 +156,10 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
smp_wmb();
  
  	/* All data has been written, so now move the offset of sub buffer. */

-   relay_reserve(guc->log.runtime.relay_chan, 
guc->log.vma->obj->base.size);
+   relay_reserve(guc->log.relay_chan, guc->log.vma->obj->base.size);
  
  	/* Switch to the next sub buffer */

-   relay_flush(guc->log.runtime.relay_chan);
+   relay_flush(guc->log.relay_chan);
  }
  
  static void *guc_get_write_buffer(struct intel_guc *guc)

@@ -173,7 +173,7 @@ static void *guc_get_write_buffer(struct intel_guc *guc)
 * done without using relay_reserve() along with relay_write(). So its
 * better to use relay_reserve() alone.
 */
-   return relay_reserve(guc->log.runtime.relay_chan, 0);
+   return relay_reserve(guc->log.relay_chan, 0);
  }
  
  static bool guc_check_log_buf_overflow(struct intel_guc *guc,

@@ -224,11 +224,11 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
void *src_data, *dst_data;
bool new_overflow;
  
-	if (WARN_ON(!guc->log.runtime.buf_addr))

+   if (WARN_ON(!guc->log.buf_addr))
return;
  
  	/* Get the pointer to shared GuC log buffer */

-   log_buf_state = src_data = guc->log.runtime.buf_addr;
+   log_buf_state = src_data = guc->log.buf_addr;
  
  	/* Get the pointer to local buffer to store the logs */

log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -316,16 +316,16 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
  static void capture_logs_work(struct work_struct *work)
  {
struct intel_guc *guc =
-   container_of(work, struct i