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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
