[Intel-gfx] [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings

[Hans de Goede]

Hi,

On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>>         iosf_mbi_punit_acquire();
>>>>>
>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>                      50)) {
>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>         }
>>>>>         iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least.

Ok, I've done a whole bunch of suspend + resume cycles on my cht
tablet with this patch dropped and things still work fine
(where as without the first 12 patches of this patch-set that
  was a guarenteed way to get a forcewake timeout followed by
  a lockup).

So it indeed seems this test is not necessary. I'll send a v3
with that patch dropped, as well as your comments for
patches 11 and 12 being addressed.

Regards,

Hans