[Intel-gfx] [PATCH 05/15] drm/i915/guc: Log runtime should consist of both mapping and relay
Sagar Arun Kamble
sagar.a.kamble at intel.com
Tue Mar 6 05:54:24 UTC 2018
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 <michal.winiarski at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> ---
>>> 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]
<snip>
> How strongly do you feel about this one?
> I wanted to tidy first (decouple things), rename later.
fine. it's ok.
>>> INIT_WORK(&guc->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(&guc->log.runtime.relay_lock);
>>> -
>>> - GEM_BUG_ON(guc_log_has_relay(guc));
>>> + lockdep_assert_held(&guc->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(&guc->log.runtime.relay_lock);
>>> -
>>> return 0;
>>> err:
>>> - mutex_unlock(&guc->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(&guc->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(&guc->log.runtime.lock);
>>> relay_close(guc->log.runtime.relay_chan);
>>> guc->log.runtime.relay_chan = NULL;
>>> -
>>> -out_unlock:
>>> - mutex_unlock(&guc->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(&guc->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(&guc->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(&dev_priv->drm.struct_mutex);
>>> - ret = guc_log_runtime_create(guc);
>>> - mutex_unlock(&dev_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(&guc->log.runtime.lock);
>>> +
>>> return 0;
>>> -err_runtime:
>>> - mutex_lock(&dev_priv->drm.struct_mutex);
>>> - guc_log_runtime_destroy(guc);
>>> - mutex_unlock(&dev_priv->drm.struct_mutex);
>>> +err_unmap:
>>> + guc_log_unmap(guc);
>>> err_relay:
>>> guc_log_relay_destroy(guc);
>>> err:
>>> + mutex_unlock(&guc->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(&dev_priv->drm.struct_mutex);
>>> - guc_log_runtime_destroy(guc);
>>> - mutex_unlock(&dev_priv->drm.struct_mutex);
>>> + mutex_lock(&guc->log.runtime.lock);
>>> +
>>> + GEM_BUG_ON(!guc_log_has_runtime(guc));
>>> + guc_log_unmap(guc);
>>> guc_log_relay_destroy(guc);
>>> +
>>> + mutex_unlock(&guc->log.runtime.lock);
>>> }
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
>>> index 09dd2ef1933d..8c26cce77a98 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_log.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
>>> @@ -48,8 +48,7 @@ struct intel_guc_log {
>>> struct workqueue_struct *flush_wq;
>>> struct work_struct flush_work;
>>> struct rchan *relay_chan;
>>> - /* To serialize the access to relay_chan */
>> Interesting, checkpatch does not complain about comment being removed for
>> mutex lock :)
> Well, it's a member of intel_guc_log, and it protects other members of
> intel_guc_log.
Yes. I was saying that if we just define new lock without comment,
checkpatch complains but
if we remove comment of earlier defined lock it doesn't :)
> -Michał
>
>>> - struct mutex relay_lock;
>>> + struct mutex lock;
>>> } runtime;
>>> /* logging related stats */
>>> u32 capture_miss_count;
>>> 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(&dev_priv->guc);
>>> + if (i915_modparams.guc_log_level)
>>> + ret = intel_guc_log_register(&dev_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(&dev_priv->guc);
>>> + if (i915_modparams.guc_log_level)
>>> + intel_guc_log_unregister(&dev_priv->guc);
>>> }
>>> static int guc_enable_communication(struct intel_guc *guc)
>> --
>> Thanks,
>> Sagar
>>
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list