[PATCH v3 12/12] drm/i915: Listen for PMIC bus access notifications
Hans de Goede
hdegoede at redhat.com
Mon Feb 20 08:29:10 UTC 2017
Hi,
On 16-02-17 20:02, Jani Nikula wrote:
> On Thu, 16 Feb 2017, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
>> On Fri, Feb 10, 2017 at 11:28:02AM +0100, Hans de Goede wrote:
>>> Listen for PMIC bus access notifications and get FORCEWAKE_ALL while
>>> the bus is accessed to avoid needing to do any forcewakes, which need
>>> PMIC bus access, while the PMIC bus is busy:
>>>
>>> This fixes errors like these showing up in dmesg, usually followed
>>> by a gfx or system freeze:
>>>
>>> [drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
>>> [drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
>>> i2c_designware 808622C1:06: punit semaphore timed out, resetting
>>> i2c_designware 808622C1:06: PUNIT SEM: 2
>>> i2c_designware 808622C1:06: couldn't acquire bus ownership
>>>
>>> Downside of this approach is that it causes wakeups whenever the PMIC
>>> bus is accessed. Unfortunately we cannot simply wait for the PMIC bus
>>> to go idle when we hit a race, as forcewakes may be done from interrupt
>>> handlers where we cannot sleep to wait for the i2c PMIC bus access to
>>> finish.
>>>
>>> Note that the notifications and thus the wakeups will only happen on
>>> baytrail / cherrytrail devices using PMICs with a shared i2c bus for
>>> P-Unit and host PMIC access (i2c busses with a _SEM method in their
>>> APCI node), e.g. an axp288 PMIC.
>>>
>>> I plan to write some patches for drivers accessing the PMIC bus to
>>> limit their bus accesses to a bare minimum (e.g. cache registers, do not
>>> update battery level more often then 4 times a minute), to limit the
>>> amount of wakeups.
>>>
>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> Tested-by: tagorereddy <tagore.chandan at gmail.com>
>>
>> I gave the previous versions a quick whirl on a few machines here, but
>> them not being CR versions I guess this stuff doesn't kick in at all.
>> And I don't see any _SEM stuff in the DSDT/SSDT, so I guess that
>> confirms it. Which is fine since I've not seem any stability issues
>> on those machines. So at least nothing seemed to break :)
>>
>> Anyways the changes look all right to me, so for both i915 patches
>> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Acked-by: Jani Nikula <jani.nikula at intel.com>
>
> for merging the i915 patches through some other tree if that makes
> managing the pile easier.
Actually the idea was for the entire pile to go through the drm-intel
tree. Daniel can you pick these up please (they seem to be ready) ?
Regards,
Hans
>
>
>>
>>> ---
>>> Changes in v2:
>>> -Spelling: P-Unit, PMIC
>>> Changes in v3:
>>> -Improve comment explaining why we call intel_uncore_forcewake_get(ALL) on
>>> MBI_PMIC_BUS_ACCESS_BEGIN notification
>>> ---
>>> drivers/gpu/drm/i915/Kconfig | 1 +
>>> drivers/gpu/drm/i915/i915_drv.h | 1 +
>>> drivers/gpu/drm/i915/intel_uncore.c | 39 +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 41 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>>> index 1ae0bb9..a5cd5da 100644
>>> --- a/drivers/gpu/drm/i915/Kconfig
>>> +++ b/drivers/gpu/drm/i915/Kconfig
>>> @@ -20,6 +20,7 @@ config DRM_I915
>>> select ACPI_VIDEO if ACPI
>>> select ACPI_BUTTON if ACPI
>>> select SYNC_FILE
>>> + select IOSF_MBI
>>> help
>>> Choose this option if you have a system that has "Intel Graphics
>>> Media Accelerator" or "HD Graphics" integrated graphics,
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 998239f..4e405d0 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -725,6 +725,7 @@ struct intel_uncore {
>>> const struct intel_forcewake_range *fw_domains_table;
>>> unsigned int fw_domains_table_entries;
>>>
>>> + struct notifier_block pmic_bus_access_nb;
>>> struct intel_uncore_funcs funcs;
>>>
>>> unsigned fifo_count;
>>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>>> index b6d726b..1be3cdc 100644
>>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>>> @@ -25,6 +25,7 @@
>>> #include "intel_drv.h"
>>> #include "i915_vgpu.h"
>>>
>>> +#include <asm/iosf_mbi.h>
>>> #include <linux/pm_runtime.h>
>>>
>>> #define FORCEWAKE_ACK_TIMEOUT_MS 50
>>> @@ -436,12 +437,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
>>>
>>> void intel_uncore_suspend(struct drm_i915_private *dev_priv)
>>> {
>>> + iosf_mbi_unregister_pmic_bus_access_notifier(
>>> + &dev_priv->uncore.pmic_bus_access_nb);
>>> intel_uncore_forcewake_reset(dev_priv, false);
>>> }
>>>
>>> void intel_uncore_resume_early(struct drm_i915_private *dev_priv)
>>> {
>>> __intel_uncore_early_sanitize(dev_priv, true);
>>> + iosf_mbi_register_pmic_bus_access_notifier(
>>> + &dev_priv->uncore.pmic_bus_access_nb);
>>> i915_check_and_clear_faults(dev_priv);
>>> }
>>>
>>> @@ -1329,6 +1334,32 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
>>> dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
>>> }
>>>
>>> +static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
>>> + unsigned long action, void *data)
>>> +{
>>> + struct drm_i915_private *dev_priv = container_of(nb,
>>> + struct drm_i915_private, uncore.pmic_bus_access_nb);
>>> +
>>> + switch (action) {
>>> + case MBI_PMIC_BUS_ACCESS_BEGIN:
>>> + /*
>>> + * forcewake all now to make sure that we don't need to do a
>>> + * forcewake later which on systems where this notifier gets
>>> + * called requires the punit to access to the shared pmic i2c
>>> + * bus, which will be busy after this notification, leading to:
>>> + * "render: timed out waiting for forcewake ack request."
>>> + * errors.
>>> + */
>>> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>> + break;
>>> + case MBI_PMIC_BUS_ACCESS_END:
>>> + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>>> + break;
>>> + }
>>> +
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> void intel_uncore_init(struct drm_i915_private *dev_priv)
>>> {
>>> i915_check_vgpu(dev_priv);
>>> @@ -1338,6 +1369,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>> __intel_uncore_early_sanitize(dev_priv, false);
>>>
>>> dev_priv->uncore.unclaimed_mmio_check = 1;
>>> + dev_priv->uncore.pmic_bus_access_nb.notifier_call =
>>> + i915_pmic_bus_access_notifier;
>>>
>>> switch (INTEL_INFO(dev_priv)->gen) {
>>> default:
>>> @@ -1392,6 +1425,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>> if (INTEL_GEN(dev_priv) >= 8)
>>> intel_shadow_table_check();
>>>
>>> + iosf_mbi_register_pmic_bus_access_notifier(
>>> + &dev_priv->uncore.pmic_bus_access_nb);
>>> +
>>> i915_check_and_clear_faults(dev_priv);
>>> }
>>> #undef ASSIGN_WRITE_MMIO_VFUNCS
>>> @@ -1399,6 +1435,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
>>>
>>> void intel_uncore_fini(struct drm_i915_private *dev_priv)
>>> {
>>> + iosf_mbi_unregister_pmic_bus_access_notifier(
>>> + &dev_priv->uncore.pmic_bus_access_nb);
>>> +
>>> /* Paranoia: make sure we have disabled everything before we exit. */
>>> intel_uncore_sanitize(dev_priv);
>>> intel_uncore_forcewake_reset(dev_priv, false);
>>> --
>>> 2.9.3
>
More information about the dri-devel
mailing list