Re: [Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-03-06 Thread Chris Wilson 
Quoting Michał Winiarski (2018-02-27 12:52:20)
> -static int guc_log_runtime_create(struct intel_guc *guc)
> +static int guc_log_map(struct intel_guc *guc)
>  {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> int ret;
>  
> -   lockdep_assert_held(_priv->drm.struct_mutex);
> -
> if (!guc->log.vma)
> return -ENODEV;
>  
> -   GEM_BUG_ON(guc_log_has_runtime(guc));
> -
> +   mutex_lock(_priv->drm.struct_mutex);
> ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
> if (ret)
> return ret;
> @@ -416,20 +390,16 @@ static int guc_log_runtime_create(struct intel_guc *guc)
> DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> return PTR_ERR(vaddr);
> }
> +   mutex_unlock(_priv->drm.struct_mutex);

Whilst you are here, we don't need struct_mutex for
i915_gem_object_pin_map(), and we don't strictly need it for
set_to_wc_domain, but I haven't replaced that with
i915_gem_object_lock() yet.

If you could restrict the locking to set_to_wc_domain (and not introduce
an unbalanced lock on error return) that would be grand.

mutex_lock(_priv->drm.struct_mutex);
ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
mutex_unlock(_priv->drm.struct_mutex);
if (ret)
return ret;
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-03-05 Thread Sagar Arun Kamble 



On 3/5/2018 7:44 PM, Michał Winiarski wrote:

On Mon, Mar 05, 2018 at 04:01:18PM +0530, Sagar Arun Kamble wrote:


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

Currently, we're treating relay and mapping of GuC log as a separate
concepts. We're also using inconsistent locking, sometimes using
relay_lock, sometimes using struct mutex.
Let's correct that. Anything touching the runtime is now serialized
using runtime.lock, while we're still using struct mutex as inner lock
for mapping.
We're still racy in setting the log level - but we'll take care of that
in the following patches.

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_log.c | 110 
++-
   drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
   drivers/gpu/drm/i915/intel_uc.c  |  14 +++--
   3 files changed, 42 insertions(+), 85 deletions(-)


[SNIP]



How strongly do you feel about this one?
I wanted to tidy first (decouple things), rename later.

fine. it's ok.

INIT_WORK(>log.runtime.flush_work, capture_logs_work);
   }
@@ -448,12 +418,7 @@ int guc_log_relay_create(struct intel_guc *guc)
size_t n_subbufs, subbuf_size;
int ret;
-   if (!i915_modparams.guc_log_level)
-   return 0;
-
-   mutex_lock(>log.runtime.relay_lock);
-
-   GEM_BUG_ON(guc_log_has_relay(guc));
+   lockdep_assert_held(>log.runtime.lock);
 /* Keep the size of sub buffers same as shared log buffer */
subbuf_size = GUC_LOG_SIZE;
@@ -483,12 +448,9 @@ int guc_log_relay_create(struct intel_guc *guc)
GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
guc->log.runtime.relay_chan = guc_log_relay_chan;
-   mutex_unlock(>log.runtime.relay_lock);
-
return 0;
   err:
-   mutex_unlock(>log.runtime.relay_lock);
/* logging will be off */
i915_modparams.guc_log_level = 0;

This log_level decoupling is not taken care

Yup, though it belongs in "drm/i915/guc: Split relay control and GuC log level",
I'll add it there.


return ret;
@@ -496,20 +458,10 @@ int guc_log_relay_create(struct intel_guc *guc)
   void guc_log_relay_destroy(struct intel_guc *guc)
   {
-   mutex_lock(>log.runtime.relay_lock);
-
-   /*
-* It's possible that the relay was never allocated because
-* GuC log was disabled at the boot time.
-*/
-   if (!guc_log_has_relay(guc))
-   goto out_unlock;
+   lockdep_assert_held(>log.runtime.lock);
relay_close(guc->log.runtime.relay_chan);
guc->log.runtime.relay_chan = NULL;
-
-out_unlock:
-   mutex_unlock(>log.runtime.relay_lock);
   }
   static void guc_log_capture_logs(struct intel_guc *guc)
@@ -608,7 +560,6 @@ static void guc_log_flush_irq_disable(struct intel_guc *guc)
   void intel_guc_log_destroy(struct intel_guc *guc)
   {
-   guc_log_runtime_destroy(guc);
i915_vma_unpin_and_release(>log.vma);
   }
@@ -678,9 +629,10 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 
val)
   int intel_guc_log_register(struct intel_guc *guc)
   {
-   struct drm_i915_private *dev_priv = guc_to_i915(guc);
int ret;
+   mutex_lock(>log.runtime.lock);
+
GEM_BUG_ON(guc_log_has_runtime(guc));
/*
@@ -692,35 +644,33 @@ int intel_guc_log_register(struct intel_guc *guc)
if (ret)
goto err;
-   mutex_lock(_priv->drm.struct_mutex);
-   ret = guc_log_runtime_create(guc);
-   mutex_unlock(_priv->drm.struct_mutex);
-
+   ret = guc_log_map(guc);
if (ret)
goto err_relay;
ret = guc_log_relay_file_create(guc);
if (ret)
-   goto err_runtime;
+   goto err_unmap;
guc_log_flush_irq_enable(guc);
+   mutex_unlock(>log.runtime.lock);
+
return 0;
-err_runtime:
-   mutex_lock(_priv->drm.struct_mutex);
-   guc_log_runtime_destroy(guc);
-   mutex_unlock(_priv->drm.struct_mutex);
+err_unmap:
+   guc_log_unmap(guc);
   err_relay:
guc_log_relay_destroy(guc);
   err:
+   mutex_unlock(>log.runtime.lock);
+
return ret;
   }
   void intel_guc_log_unregister(struct intel_guc *guc)
   {
-   struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
+   guc_log_flush_irq_disable(guc);

This move could be part of earlier patch.

/*
 * Once logging is disabled, GuC won't generate logs & send an
 * interrupt. But there could be some data in the log buffer
@@ -728,11 +678,13 @@ void intel_guc_log_unregister(struct intel_guc *guc)
 * buffer state and then collect the left over logs.
 */
guc_flush_logs(guc);
-   guc_log_flush_irq_disable(guc);
-   mutex_lock(_priv->drm.struct_mutex);
-

Re: [Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-03-05 Thread Michał Winiarski 
On Mon, Mar 05, 2018 at 04:01:18PM +0530, Sagar Arun Kamble wrote:
> 
> 
> On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> > Currently, we're treating relay and mapping of GuC log as a separate
> > concepts. We're also using inconsistent locking, sometimes using
> > relay_lock, sometimes using struct mutex.
> > Let's correct that. Anything touching the runtime is now serialized
> > using runtime.lock, while we're still using struct mutex as inner lock
> > for mapping.
> > We're still racy in setting the log level - but we'll take care of that
> > in the following patches.
> > 
> > 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_log.c | 110 
> > ++-
> >   drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
> >   drivers/gpu/drm/i915/intel_uc.c  |  14 +++--
> >   3 files changed, 42 insertions(+), 85 deletions(-)
> > 

[SNIP]

> > @@ -372,8 +349,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
> > *guc)
> > }
> > guc_move_to_next_buf(guc);
> > -
> > -   mutex_unlock(>log.runtime.relay_lock);
> >   }
> >   static void capture_logs_work(struct work_struct *work)
> > @@ -381,7 +356,9 @@ static void capture_logs_work(struct work_struct *work)
> > struct intel_guc *guc =
> > container_of(work, struct intel_guc, log.runtime.flush_work);
> > +   mutex_lock(>log.runtime.lock);
> > guc_log_capture_logs(guc);
> I think we should just lock guc_read_update_log_buffer as
> guc_log_flush_complete is independent action

Agreed - I'll change this and apply all the other suggestions to the locking.

> > +   mutex_unlock(>log.runtime.lock);
> >   }
> >   static bool guc_log_has_runtime(struct intel_guc *guc)
> > @@ -389,19 +366,16 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
> > return guc->log.runtime.buf_addr != NULL;
> >   }
> > -static int guc_log_runtime_create(struct intel_guc *guc)
> > +static int guc_log_map(struct intel_guc *guc)
> >   {
> > struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > void *vaddr;
> > int ret;
> > -   lockdep_assert_held(_priv->drm.struct_mutex);
> > -
> lockdep_assert for runtime.lock here?
> > if (!guc->log.vma)
> > return -ENODEV;
> > -   GEM_BUG_ON(guc_log_has_runtime(guc));
> > -
> > +   mutex_lock(_priv->drm.struct_mutex);
> > ret = i915_gem_object_set_to_wc_domain(guc->log.vma->obj, true);
> > if (ret)
> mutex not unlocked
> > return ret;
> > @@ -416,20 +390,16 @@ static int guc_log_runtime_create(struct intel_guc 
> > *guc)
> > DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> mutex not unlocked
> > return PTR_ERR(vaddr);
> > }
> > +   mutex_unlock(_priv->drm.struct_mutex);
> > guc->log.runtime.buf_addr = vaddr;
> > return 0;
> >   }
> > -static void guc_log_runtime_destroy(struct intel_guc *guc)
> > +static void guc_log_unmap(struct intel_guc *guc)
> >   {
> > -   /*
> > -* It's possible that the runtime stuff was never allocated because
> > -* GuC log was disabled at the boot time.
> > -*/
> > -   if (!guc_log_has_runtime(guc))
> > -   return;
> > +   lockdep_assert_held(>log.runtime.lock);
> struct_mutex locking here?

Except this one. AFACT, we don't really need struct mutex here. We need it only
for set_domain - I'll reduce the scope to set_domain on the map path.

> > i915_gem_object_unpin_map(guc->log.vma->obj);
> > guc->log.runtime.buf_addr = NULL;
> > @@ -437,7 +407,7 @@ static void guc_log_runtime_destroy(struct intel_guc 
> > *guc)
> >   void intel_guc_log_init_early(struct intel_guc *guc)
> >   {
> > -   mutex_init(>log.runtime.relay_lock);
> > +   mutex_init(>log.runtime.lock);
> rename and move of members from runtime to log can precede this patch?

How strongly do you feel about this one?
I wanted to tidy first (decouple things), rename later.

> > INIT_WORK(>log.runtime.flush_work, capture_logs_work);
> >   }
> > @@ -448,12 +418,7 @@ int guc_log_relay_create(struct intel_guc *guc)
> > size_t n_subbufs, subbuf_size;
> > int ret;
> > -   if (!i915_modparams.guc_log_level)
> > -   return 0;
> > -
> > -   mutex_lock(>log.runtime.relay_lock);
> > -
> > -   GEM_BUG_ON(guc_log_has_relay(guc));
> > +   lockdep_assert_held(>log.runtime.lock);
> >  /* Keep the size of sub buffers same as shared log buffer */
> > subbuf_size = GUC_LOG_SIZE;
> > @@ -483,12 +448,9 @@ int guc_log_relay_create(struct intel_guc *guc)
> > GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size);
> > guc->log.runtime.relay_chan = guc_log_relay_chan;
> > -   mutex_unlock(>log.runtime.relay_lock);
> > -
> > return 0;
> >   err:
> > -   mutex_unlock(>log.runtime.relay_lock);
> >

Re: [Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-03-05 Thread Sagar Arun Kamble 



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

Currently, we're treating relay and mapping of GuC log as a separate
concepts. We're also using inconsistent locking, sometimes using
relay_lock, sometimes using struct mutex.
Let's correct that. Anything touching the runtime is now serialized
using runtime.lock, while we're still using struct mutex as inner lock
for mapping.
We're still racy in setting the log level - but we'll take care of that
in the following patches.

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_log.c | 110 ++-
  drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
  drivers/gpu/drm/i915/intel_uc.c  |  14 +++--
  3 files changed, 42 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
b/drivers/gpu/drm/i915/intel_guc_log.c
index 09be7340652b..567620464f52 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -153,10 +153,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
struct dentry *log_dir;
int ret;
  
-	if (!i915_modparams.guc_log_level)

-   return 0;
-
-   mutex_lock(>log.runtime.relay_lock);
+   lockdep_assert_held(>log.runtime.lock);
  
  	/* For now create the log file in /sys/kernel/debug/dri/0 dir */

log_dir = dev_priv->drm.primary->debugfs_root;
@@ -175,28 +172,16 @@ static int guc_log_relay_file_create(struct intel_guc 
*guc)
 */
if (!log_dir) {
DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-   ret = -ENODEV;
-   goto out_unlock;
+   return -ENODEV;
}
  
  	ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", log_dir);

if (ret < 0 && ret != -EEXIST) {
DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
-   goto out_unlock;
+   return ret;
}
  
-	ret = 0;

-
-out_unlock:
-   mutex_unlock(>log.runtime.relay_lock);
-   return ret;
-}
-
-static bool guc_log_has_relay(struct intel_guc *guc)
-{
-   lockdep_assert_held(>log.runtime.relay_lock);
-
-   return guc->log.runtime.relay_chan != NULL;
+   return 0;
  }
  
  static void guc_move_to_next_buf(struct intel_guc *guc)

@@ -207,9 +192,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 */
smp_wmb();
  
-	if (!guc_log_has_relay(guc))

-   return;
-
/* 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);
  
@@ -219,9 +201,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
  
  static void *guc_get_write_buffer(struct intel_guc *guc)

  {
-   if (!guc_log_has_relay(guc))
-   return NULL;
-
/*
 * Just get the base address of a new sub buffer and copy data into it
 * ourselves. NULL will be returned in no-overwrite mode, if all sub
@@ -288,7 +267,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
/* Get the pointer to shared GuC log buffer */
log_buf_state = src_data = guc->log.runtime.buf_addr;
  
-	mutex_lock(>log.runtime.relay_lock);
  
  	/* Get the pointer to local buffer to store the logs */

log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -300,7 +278,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
 */
DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
guc->log.capture_miss_count++;
-   mutex_unlock(>log.runtime.relay_lock);
  
  		return;

}
@@ -372,8 +349,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
}
  
  	guc_move_to_next_buf(guc);

-
-   mutex_unlock(>log.runtime.relay_lock);
  }
  
  static void capture_logs_work(struct work_struct *work)

@@ -381,7 +356,9 @@ static void capture_logs_work(struct work_struct *work)
struct intel_guc *guc =
container_of(work, struct intel_guc, log.runtime.flush_work);
  
+	mutex_lock(>log.runtime.lock);

guc_log_capture_logs(guc);
I think we should just lock guc_read_update_log_buffer as 
guc_log_flush_complete is independent action

+   mutex_unlock(>log.runtime.lock);
  }
  
  static bool guc_log_has_runtime(struct intel_guc *guc)

@@ -389,19 +366,16 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
return guc->log.runtime.buf_addr != NULL;
  }
  
-static int guc_log_runtime_create(struct intel_guc *guc)

+static int guc_log_map(struct intel_guc *guc)
  {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
int ret;

Re: [Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-02-27 Thread Michal Wajdeczko 
On Tue, 27 Feb 2018 13:52:20 +0100, Michał Winiarski  
 wrote:



Currently, we're treating relay and mapping of GuC log as a separate
concepts. We're also using inconsistent locking, sometimes using
relay_lock, sometimes using struct mutex.
Let's correct that. Anything touching the runtime is now serialized
using runtime.lock, while we're still using struct mutex as inner lock
for mapping.
We're still racy in setting the log level - but we'll take care of that
in the following patches.

Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Michal Wajdeczko 
---


/snip/

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

index e9aba3c35264..55a9b5b673e0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -221,18 +221,24 @@ static void guc_free_load_err_log(struct intel_guc  
*guc)

int intel_uc_log_register(struct drm_i915_private *dev_priv)
 {
-   if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
+   int ret = 0;
+
+   if (!USES_GUC(dev_priv))
return 0;
-   return intel_guc_log_register(_priv->guc);
+   if (i915_modparams.guc_log_level)
+   ret = intel_guc_log_register(_priv->guc);
+
+   return ret;
 }
void intel_uc_log_unregister(struct drm_i915_private *dev_priv)
 {
-   if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
+   if (!USES_GUC(dev_priv))
return;
-   intel_guc_log_unregister(_priv->guc);
+   if (i915_modparams.guc_log_level)
+   intel_guc_log_unregister(_priv->guc);
 }
static int guc_enable_communication(struct intel_guc *guc)


this chunk can be done in patch 2/15
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay

2018-02-27 Thread Michał Winiarski 
Currently, we're treating relay and mapping of GuC log as a separate
concepts. We're also using inconsistent locking, sometimes using
relay_lock, sometimes using struct mutex.
Let's correct that. Anything touching the runtime is now serialized
using runtime.lock, while we're still using struct mutex as inner lock
for mapping.
We're still racy in setting the log level - but we'll take care of that
in the following patches.

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_log.c | 110 ++-
 drivers/gpu/drm/i915/intel_guc_log.h |   3 +-
 drivers/gpu/drm/i915/intel_uc.c  |  14 +++--
 3 files changed, 42 insertions(+), 85 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c 
b/drivers/gpu/drm/i915/intel_guc_log.c
index 09be7340652b..567620464f52 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -153,10 +153,7 @@ static int guc_log_relay_file_create(struct intel_guc *guc)
struct dentry *log_dir;
int ret;
 
-   if (!i915_modparams.guc_log_level)
-   return 0;
-
-   mutex_lock(>log.runtime.relay_lock);
+   lockdep_assert_held(>log.runtime.lock);
 
/* For now create the log file in /sys/kernel/debug/dri/0 dir */
log_dir = dev_priv->drm.primary->debugfs_root;
@@ -175,28 +172,16 @@ static int guc_log_relay_file_create(struct intel_guc 
*guc)
 */
if (!log_dir) {
DRM_ERROR("Debugfs dir not available yet for GuC log file\n");
-   ret = -ENODEV;
-   goto out_unlock;
+   return -ENODEV;
}
 
ret = relay_late_setup_files(guc->log.runtime.relay_chan, "guc_log", 
log_dir);
if (ret < 0 && ret != -EEXIST) {
DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
-   goto out_unlock;
+   return ret;
}
 
-   ret = 0;
-
-out_unlock:
-   mutex_unlock(>log.runtime.relay_lock);
-   return ret;
-}
-
-static bool guc_log_has_relay(struct intel_guc *guc)
-{
-   lockdep_assert_held(>log.runtime.relay_lock);
-
-   return guc->log.runtime.relay_chan != NULL;
+   return 0;
 }
 
 static void guc_move_to_next_buf(struct intel_guc *guc)
@@ -207,9 +192,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 */
smp_wmb();
 
-   if (!guc_log_has_relay(guc))
-   return;
-
/* 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);
 
@@ -219,9 +201,6 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
 
 static void *guc_get_write_buffer(struct intel_guc *guc)
 {
-   if (!guc_log_has_relay(guc))
-   return NULL;
-
/*
 * Just get the base address of a new sub buffer and copy data into it
 * ourselves. NULL will be returned in no-overwrite mode, if all sub
@@ -288,7 +267,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
/* Get the pointer to shared GuC log buffer */
log_buf_state = src_data = guc->log.runtime.buf_addr;
 
-   mutex_lock(>log.runtime.relay_lock);
 
/* Get the pointer to local buffer to store the logs */
log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
@@ -300,7 +278,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
 */
DRM_ERROR_RATELIMITED("no sub-buffer to capture logs\n");
guc->log.capture_miss_count++;
-   mutex_unlock(>log.runtime.relay_lock);
 
return;
}
@@ -372,8 +349,6 @@ static void guc_read_update_log_buffer(struct intel_guc 
*guc)
}
 
guc_move_to_next_buf(guc);
-
-   mutex_unlock(>log.runtime.relay_lock);
 }
 
 static void capture_logs_work(struct work_struct *work)
@@ -381,7 +356,9 @@ static void capture_logs_work(struct work_struct *work)
struct intel_guc *guc =
container_of(work, struct intel_guc, log.runtime.flush_work);
 
+   mutex_lock(>log.runtime.lock);
guc_log_capture_logs(guc);
+   mutex_unlock(>log.runtime.lock);
 }
 
 static bool guc_log_has_runtime(struct intel_guc *guc)
@@ -389,19 +366,16 @@ static bool guc_log_has_runtime(struct intel_guc *guc)
return guc->log.runtime.buf_addr != NULL;
 }
 
-static int guc_log_runtime_create(struct intel_guc *guc)
+static int guc_log_map(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
int ret;
 
-   lockdep_assert_held(_priv->drm.struct_mutex);
-
if (!guc->log.vma)
return -ENODEV;
 
-