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

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@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

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