Re: [PATCH v2 01/11] drm/i915: Disable preemption and sleeping while using the punit sideband

From: Hans de Goede <hdegoede@redhat.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 01/11] drm/i915: Disable preemption and sleeping while using the punit sideband
Date: Mon, 15 Jan 2018 14:07:07 +0100	[thread overview]
Message-ID: <f311ec3b-7068-0737-eb84-5f6c47dc8bcc@redhat.com> (raw)
In-Reply-To: <151601890352.8139.3429022529145122646@mail.alporthouse.com>

Hi,

On 15-01-18 13:21, Chris Wilson wrote:
> Quoting Mika Kuoppala (2018-01-15 12:04:40)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>>
>>> While we talk to the punit over its sideband, we need to prevent the cpu
>>> from sleeping in order to prevent a potential machine hang.
>>>
>>> Note that by itself, it appears that pm_qos_update_request (via
>>> intel_idle) doesn't provide a sufficient barrier to ensure that all core
>>> are indeed awake (out of Cstate) and that the package is awake. To do so,
>>> we need to supplement the pm_qos with a manual ping on_each_cpu.
>>>
>>> v2: Restrict the heavy-weight wakeup to just the ISOF_PORT_PUNIT, there
>>> is insufficient evidence to implicate a wider problem atm. Similarly,
>>> restrict the w/a to Valleyview, as Cherryview doesn't have an angry cadre
>>> of users.
>>>
>>
>> One datapoint about the v1 of this patch with the cpu ping in it.
>> The j1900 byt did end up with system hang during weekend with
>> drm-tip + v1.
> 
> Question: did you set CONFIG_I2C_DESIGNWARE_BAYTRAIL=y?

Note this only matters (AFAIK) on a board with an AXP288 PMIC, on
other boards the i2c-bus is not shared between the kernel and the
punit, so no synchronization is happening (or necessary). I'm not
aware of any J1900 boards with the AXP288 (which does not mean
they do not exist).

If you have CONFIG_I2C_DESIGNWARE_BAYTRAIL enabled and you've an
affected PMIC then dmesg should contain:

"I2C bus managed by PUNIT"

Chris, are you seeing actual testing differences on a system
without a PUNIT manages I2C bus ? If so then the only reason
I can think of this is because the iosf_mbi_punit_acquire()
call will synchronize punit sideband accesses with
intel_uncore_forcewake_reset() calls, since both take the
mutex behind it, which is otherwise unused on systems without
the "I2C bus managed by PUNIT" thingie. But that would be strange
as I would not expect intel_uncore_forcewake_reset() calls
to happen during normal use (except for suspend/resume).

So if you do not have that message; and you do see different
testing results from adding the iosf_mbi_punit_acquire(), then
that suggests that we need to make sure no punit sideband
accesses are happening while changing forcewake settings.

iosf_mbi_punit_acquire() is not a complete solution here,
the i2c-designware-baytrail code also calls a notifier
(iosf_mbi_call_pmic_bus_access_notifier_chain) before doing
a PMIC access over the shared i2c bus, i915 code listens to
that notifier and gets all forcewake bits before before
the notifier call returns, so that we can be sure there will
be no forcewake gets/puts happening while an i2c bus access
is active on the PMIC i2c bus shared with the PUNIT.

The notifier approach is used here because the forcewake
gets may happen from non-sleeping contexts, so we just make
sure that everything is active beforehand.

With all this said I still think that doing the
iosf_mbi_punit_acquire() is a good idea, so that we block
i2c transfers on the PMIC i2c bus shared with the PUNIT
while we are talking to the PUNIT.

Regards,

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