Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Deepak S <deepak.s@intel.com>]

On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
> On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
>> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
>>> On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
>>>> From: Deepak S <deepak.s@linux.intel.com>
>>>>
>>>> In chv, we have two power wells Render & Media. We need to use
>>>> corresponsing forcewake count. If we dont follow this we are getting
>>>> error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
>>>> multiple entry into __vlv_force_wake_get.
>>>>
>>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
>>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index bd1b28d..bafd38b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>>>>   	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>>>>   	 */
>>>>   	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>>>> -	if (dev_priv->uncore.forcewake_count++ == 0)
>>>> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
>>>> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
>>>> +		if (dev_priv->uncore.fw_rendercount++ == 0)
>>>> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>>>> +							      FORCEWAKE_RENDER);
>>>> +		if (dev_priv->uncore.fw_mediacount++ == 0)
>>>> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>>>> +							      FORCEWAKE_MEDIA);
>>> This will wake both wells. Is that needed or should we just pick one
>>> based on the ring?
>> Also unlike the comment says runtime_pm_get() can't sleep since someone
>> must already be holding a reference, othwewise we surely can't go
>> writing any registers. So in theory we should be able to call
>> gen6_gt_force_wake_get() here, but maybe that would trigger a
>> might_sleep() warning. the current force wake code duplication (esp.
>> outside intel_uncore.c) is rather unfortunate and I'd like to see it
>> killed off. Maybe we just need to pull the rpm get/put outside
>> gen6_gt_force_wake_get()? I never really liked hiding it there anyway.
> Yeah this is just broken design. And if you look at the other wheel to
> track outstanding gpu work (requests) then it's not needed at all.
>
> But I'm not sure what's the priority of the "rework execlists to use
> requests" task is and when (if ever that will happen). Jesse is the
> arbiter for this stuff anyway, so adding him.
> -Daniel

hmm , agreed do we have a reworked execlist? The reason why added this, on chv when i enable execlist, due to incorrect forcewake count
is causing multiple entries to forcewake_get resulting in "*ERROR*: Timed out waiting for forcewake old ack to clear" "and Hang.