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

[Ville Syrjälä]

On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
> 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. It also makes me wonder if
> > just bumping the timeouts to some ridiculously high value would fix
> > the problem as well.
> 
> I've already tried bumping the forcewake timeout from 50 to 250ms,
> before writing the patch to just get forcewake_all before the pmic
> bus access begins, that does not fix things,

And you bumped the i2c mutex timeout as well? Or does that fail somehow
gracefully if it can't get the mutex?

> and since we busy wait
> for this timeout from non-sleeping context 250ms already is way too
> high.

Sure, but I'm just trying to understand if the problem is simply caused
by proceeding with some hardware access without getting the i2c mutex.

> 
> >>>>>> + a comment would be nice why it's there.
> >>>>>
> >>>>> I will add comments to the acquire calls.
> >>>>>
> >>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>>>> ifdefs?
> >>>>>
> >>>>> No, the iosf_mbi header defines empty inline versions of
> >>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>>>> this does mean that iosf_mbi must be builtin if the i915
> >>>>> driver is. I'll add:
> >>>>>
> >>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>>>
> >>>>> To the i915 Kconfig to enforce this.
> >>>>
> >>>> Hmm, ok so that does not work (long cyclic dependency through the
> >>>> selection of ACPI_VIDEO).
> >>>>
> >>>> So I've now added this instead:
> >>>>
> >>>> 	# iosf_mbi needs to be builtin if we are builtin
> >>>> 	select IOSF_MBI if DRM_I915=y
> >>>
> >>> That's probably not going to help anyone since i915 is usually a module.
> >>
> >> Right, that is fine, then either the IOSF_MBI symbols are available,
> >> or IOSF_MBI is disabled and we get the inline nops from the header.
> >>
> >> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> >> realistic IMHO, but will get triggered by the random-config testing
> >> several contributors do and lead to an unresolved symbol error there.
> >
> > Well, from the user POV anything with IOSF_MBI==n can be a problem.
> > So I'm not sure if we should allow that.
> 
> So you're suggesting we just add an unconditional "select IOSF_MBI"
> to the i915 Kconfig entry?

Yeah, that should at least cut down the number of people accidentally
misconfiguring their kernels and hitting this problem in the future.

-- 
Ville Syrjälä
Intel OTC