* [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses
@ 2017-01-01 20:13 Hans de Goede
2017-01-01 20:14 ` [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access Hans de Goede
` (3 more replies)
0 siblings, 4 replies; 25+ messages in thread
From: Hans de Goede @ 2017-01-01 20:13 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
Hi,
So one user is reporting problems with my patches to get the
i2c pmic bus (cherrytrail punit semaphore support) + axp288 fuel_gauge /
charger drivers working in combination with i915:
https://bugzilla.kernel.org/show_bug.cgi?id=155241#c37
"My device is a laptop with no USB charging or OTG. So, I'd tried only SDIO _ADR patch, i2c and axp288_fuel_gauge patches. Everything works well for upto 3 mins after boot, then the device freezes. I hadn't tried any drm patches BTW. Here's the log:
[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.
[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.
clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
clocksource: 'refined-jiffies' wd_now: 10002ee30 wd_last: 10002edb8 mask: ffffffff
clocksource: 'tsc' cs_now: 16ac2c7744a cs_last: 16a8d9bd8f2 mask: ffffffffffffffff
clocksource: Switched to clocksource refined-jiffies
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
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
axp288_fuel_gauge axp288_fuel_gauge: axp288 reg read err:-110
axp288_fuel_gauge axp288_fuel_gauge: PWR STAT read failed:-110
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 0
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: IIO channel read error: fffffffb, 0
power_supply axp288_fuel_gauge: driver failed to report `voltage_now' property: -5
***SYSTEM FREEZE***
If I blacklist axp288_fuel_gauge, then there were no errors."
This sounds a lot like things go wrong when the i915 driver asks some
changes from the punit while i2c-designware-baytrail is holding the
pmic i2c bus semaphore, just like changing C-states while holding
the semaphore seems to lock up the punit,
So this patch-set is an attempt at fixing that. I'm still waiting for the
reporter to report back if it actually fixes things, in the mean time
any input on this issue (or the proposed fix) is welcome.
Note the i2c-designware-baytrail patch applies on top of v5 of my
i2c-designware series, which I will send out directly after this.
Regards,
Hans
^ permalink raw reply [flat|nested] 25+ messages in thread* [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-01 20:13 [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses Hans de Goede @ 2017-01-01 20:14 ` Hans de Goede 2017-01-02 14:12 ` Ville Syrjälä 2017-01-01 20:14 ` [RFC 2/4] i2c: designware-baytrail: Take punit lock on bus acquire Hans de Goede ` (2 subsequent siblings) 3 siblings, 1 reply; 25+ messages in thread From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw) To: Jarkko Nikula, Len Brown Cc: Jani Nikula, Ville Syrjälä, russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede The punit on baytrail / cherrytrail systems is not only accessed through the iosf_mbi functions, but also by the i915 code. Add a mutex to protect the punit against simultaneous accesses and 2 functions to lock / unlock this mutex. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++ arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ 2 files changed, 32 insertions(+) diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h index b41ee16..02963bd 100644 --- a/arch/x86/include/asm/iosf_mbi.h +++ b/arch/x86/include/asm/iosf_mbi.h @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr); */ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); +/** + * iosf_mbi_punit_lock() - Lock the punit mutex + * + * This function must be called before accessing the punit or the pmic, be it + * through iosf_mbi_* or through other means. + * + * This function locks a mutex, as such it may sleep. + */ +void iosf_mbi_punit_lock(void); + +/** + * iosf_mbi_punit_unlock() - Unlock the punit mutex + */ +void iosf_mbi_punit_unlock(void); + #else /* CONFIG_IOSF_MBI is not enabled */ static inline bool iosf_mbi_available(void) @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask) WARN(1, "IOSF_MBI driver not available"); return -EPERM; } + +static inline void iosf_mbi_punit_lock(void) {} +static inline void iosf_mbi_punit_unlock(void) {} + #endif /* CONFIG_IOSF_MBI */ #endif /* IOSF_MBI_SYMS_H */ diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c index edf2c54..75d8135 100644 --- a/arch/x86/platform/intel/iosf_mbi.c +++ b/arch/x86/platform/intel/iosf_mbi.c @@ -34,6 +34,7 @@ static struct pci_dev *mbi_pdev; static DEFINE_SPINLOCK(iosf_mbi_lock); +static DEFINE_MUTEX(iosf_mbi_punit_mutex); static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) { @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) } EXPORT_SYMBOL(iosf_mbi_available); +void iosf_mbi_punit_lock(void) +{ + mutex_lock(&iosf_mbi_punit_mutex); +} +EXPORT_SYMBOL(iosf_mbi_punit_lock); + +void iosf_mbi_punit_unlock(void) +{ + mutex_unlock(&iosf_mbi_punit_mutex); +} +EXPORT_SYMBOL(iosf_mbi_punit_unlock); + #ifdef CONFIG_IOSF_MBI_DEBUG static u32 dbg_mdr; static u32 dbg_mcr; -- 2.9.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-01 20:14 ` [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access Hans de Goede @ 2017-01-02 14:12 ` Ville Syrjälä 2017-01-02 14:21 ` Hans de Goede 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-02 14:12 UTC (permalink / raw) To: Hans de Goede Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c, Len Brown On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: > The punit on baytrail / cherrytrail systems is not only accessed through > the iosf_mbi functions, but also by the i915 code. Add a mutex to protect > the punit against simultaneous accesses and 2 functions to lock / unlock > this mutex. I'm not sure which part of punit you're actually trying to protect here. Some specific registers? > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++ > arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h > index b41ee16..02963bd 100644 > --- a/arch/x86/include/asm/iosf_mbi.h > +++ b/arch/x86/include/asm/iosf_mbi.h > @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr); > */ > int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); > > +/** > + * iosf_mbi_punit_lock() - Lock the punit mutex > + * > + * This function must be called before accessing the punit or the pmic, be it > + * through iosf_mbi_* or through other means. > + * > + * This function locks a mutex, as such it may sleep. > + */ > +void iosf_mbi_punit_lock(void); > + > +/** > + * iosf_mbi_punit_unlock() - Unlock the punit mutex > + */ > +void iosf_mbi_punit_unlock(void); > + > #else /* CONFIG_IOSF_MBI is not enabled */ > static inline > bool iosf_mbi_available(void) > @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask) > WARN(1, "IOSF_MBI driver not available"); > return -EPERM; > } > + > +static inline void iosf_mbi_punit_lock(void) {} > +static inline void iosf_mbi_punit_unlock(void) {} > + > #endif /* CONFIG_IOSF_MBI */ > > #endif /* IOSF_MBI_SYMS_H */ > diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c > index edf2c54..75d8135 100644 > --- a/arch/x86/platform/intel/iosf_mbi.c > +++ b/arch/x86/platform/intel/iosf_mbi.c > @@ -34,6 +34,7 @@ > > static struct pci_dev *mbi_pdev; > static DEFINE_SPINLOCK(iosf_mbi_lock); > +static DEFINE_MUTEX(iosf_mbi_punit_mutex); > > static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) > { > @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) > } > EXPORT_SYMBOL(iosf_mbi_available); > > +void iosf_mbi_punit_lock(void) > +{ > + mutex_lock(&iosf_mbi_punit_mutex); > +} > +EXPORT_SYMBOL(iosf_mbi_punit_lock); > + > +void iosf_mbi_punit_unlock(void) > +{ > + mutex_unlock(&iosf_mbi_punit_mutex); > +} > +EXPORT_SYMBOL(iosf_mbi_punit_unlock); > + > #ifdef CONFIG_IOSF_MBI_DEBUG > static u32 dbg_mdr; > static u32 dbg_mcr; > -- > 2.9.3 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-02 14:12 ` Ville Syrjälä @ 2017-01-02 14:21 ` Hans de Goede 2017-01-13 9:26 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Hans de Goede @ 2017-01-02 14:21 UTC (permalink / raw) To: Ville Syrjälä Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c, Len Brown Hi, On 02-01-17 15:12, Ville Syrjälä wrote: > On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: >> The punit on baytrail / cherrytrail systems is not only accessed through >> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect >> the punit against simultaneous accesses and 2 functions to lock / unlock >> this mutex. > > I'm not sure which part of punit you're actually trying to protect > here. Some specific registers? The theory I'm going by is that for certain actions / certain requests we send to the punit, the punit needs to access the (axp288) pmic, to change (or enable / disable) certain voltages. So it needs to access the pmic i2c bus, but in some cases the kernel is accessing this itself (e.g. for battery monitoring) and is holding the punit i2c bus semaphore. At least with CPU-core C-state transitions, this seems to be happening, if I do read i2c transfers on the pmic i2c bus repeatedly without blocking the CPU from entering C6 (*) while accessing the i2c bus my cherrytrail tablet will freeze in 10 - 30 seconds. The findings of one of the users commenting in: https://bugzilla.kernel.org/show_bug.cgi?id=155241 Seem to indicate a similar problem with the i915 driver doing power-management while the i2c-designware-baytrail code is holding the punit i2c bus semaphore. One would hope that the punit would be smart enough to simply wait for the semaphore to get released in that case, but at least for the C6 CPU core transition it seems that allowing that to happen while holding the semaphore causes a hard crash of the SoC. So I guess that for explicit requests the punit code assumes that the OS is not holding the semaphore. Regards, Hans *) which powers off most of the core, so likely causes interaction with the pmic > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++ >> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h >> index b41ee16..02963bd 100644 >> --- a/arch/x86/include/asm/iosf_mbi.h >> +++ b/arch/x86/include/asm/iosf_mbi.h >> @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr); >> */ >> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); >> >> +/** >> + * iosf_mbi_punit_lock() - Lock the punit mutex >> + * >> + * This function must be called before accessing the punit or the pmic, be it >> + * through iosf_mbi_* or through other means. >> + * >> + * This function locks a mutex, as such it may sleep. >> + */ >> +void iosf_mbi_punit_lock(void); >> + >> +/** >> + * iosf_mbi_punit_unlock() - Unlock the punit mutex >> + */ >> +void iosf_mbi_punit_unlock(void); >> + >> #else /* CONFIG_IOSF_MBI is not enabled */ >> static inline >> bool iosf_mbi_available(void) >> @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask) >> WARN(1, "IOSF_MBI driver not available"); >> return -EPERM; >> } >> + >> +static inline void iosf_mbi_punit_lock(void) {} >> +static inline void iosf_mbi_punit_unlock(void) {} >> + >> #endif /* CONFIG_IOSF_MBI */ >> >> #endif /* IOSF_MBI_SYMS_H */ >> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c >> index edf2c54..75d8135 100644 >> --- a/arch/x86/platform/intel/iosf_mbi.c >> +++ b/arch/x86/platform/intel/iosf_mbi.c >> @@ -34,6 +34,7 @@ >> >> static struct pci_dev *mbi_pdev; >> static DEFINE_SPINLOCK(iosf_mbi_lock); >> +static DEFINE_MUTEX(iosf_mbi_punit_mutex); >> >> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) >> { >> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) >> } >> EXPORT_SYMBOL(iosf_mbi_available); >> >> +void iosf_mbi_punit_lock(void) >> +{ >> + mutex_lock(&iosf_mbi_punit_mutex); >> +} >> +EXPORT_SYMBOL(iosf_mbi_punit_lock); >> + >> +void iosf_mbi_punit_unlock(void) >> +{ >> + mutex_unlock(&iosf_mbi_punit_mutex); >> +} >> +EXPORT_SYMBOL(iosf_mbi_punit_unlock); >> + >> #ifdef CONFIG_IOSF_MBI_DEBUG >> static u32 dbg_mdr; >> static u32 dbg_mcr; >> -- >> 2.9.3 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-02 14:21 ` Hans de Goede @ 2017-01-13 9:26 ` Ville Syrjälä 2017-01-13 10:34 ` Jarkko Nikula 2017-01-13 16:06 ` Hans de Goede 0 siblings, 2 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-01-13 9:26 UTC (permalink / raw) To: Hans de Goede Cc: Jarkko Nikula, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote: > Hi, > > On 02-01-17 15:12, Ville Syrjälä wrote: > > On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: > >> The punit on baytrail / cherrytrail systems is not only accessed through > >> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect > >> the punit against simultaneous accesses and 2 functions to lock / unlock > >> this mutex. > > > > I'm not sure which part of punit you're actually trying to protect > > here. Some specific registers? > > The theory I'm going by is that for certain actions / certain requests > we send to the punit, the punit needs to access the (axp288) pmic, to > change (or enable / disable) certain voltages. At least for cpu/display/gt voltages that shouldn't really be the case. The vcc/vnn/vgg rails are controlled via svid, not i2c. It also feels quite hand wavy since the punit could do whatever at any time AFAIK. Eg. if there's some thermal event or something the punit might kick into action. So trying to protect this from the OS side might not be able to avoid these problems entirely. It feels like there really should be some kind of shared hardware/firmware mutex with the punit to arbitrate access to the i2c bus. > So it needs to access the pmic i2c bus, but in some cases the kernel > is accessing this itself (e.g. for battery monitoring) and is holding > the punit i2c bus semaphore. At least with CPU-core C-state transitions, > this seems to be happening, if I do read i2c transfers on the pmic > i2c bus repeatedly without blocking the CPU from entering C6 (*) while > accessing the i2c bus my cherrytrail tablet will freeze in 10 - 30 > seconds. > > The findings of one of the users commenting in: > > https://bugzilla.kernel.org/show_bug.cgi?id=155241 > > Seem to indicate a similar problem with the i915 driver doing > power-management while the i2c-designware-baytrail code is holding > the punit i2c bus semaphore. One would hope that the punit would be > smart enough to simply wait for the semaphore to get released in that > case, but at least for the C6 CPU core transition it seems that allowing > that to happen while holding the semaphore causes a hard crash of the > SoC. So I guess that for explicit requests the punit code assumes that > the OS is not holding the semaphore. > > Regards, > > Hans > > > > *) which powers off most of the core, so likely causes interaction with > the pmic > > > > > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++ > >> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++ > >> 2 files changed, 32 insertions(+) > >> > >> diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h > >> index b41ee16..02963bd 100644 > >> --- a/arch/x86/include/asm/iosf_mbi.h > >> +++ b/arch/x86/include/asm/iosf_mbi.h > >> @@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr); > >> */ > >> int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask); > >> > >> +/** > >> + * iosf_mbi_punit_lock() - Lock the punit mutex > >> + * > >> + * This function must be called before accessing the punit or the pmic, be it > >> + * through iosf_mbi_* or through other means. > >> + * > >> + * This function locks a mutex, as such it may sleep. > >> + */ > >> +void iosf_mbi_punit_lock(void); > >> + > >> +/** > >> + * iosf_mbi_punit_unlock() - Unlock the punit mutex > >> + */ > >> +void iosf_mbi_punit_unlock(void); > >> + > >> #else /* CONFIG_IOSF_MBI is not enabled */ > >> static inline > >> bool iosf_mbi_available(void) > >> @@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask) > >> WARN(1, "IOSF_MBI driver not available"); > >> return -EPERM; > >> } > >> + > >> +static inline void iosf_mbi_punit_lock(void) {} > >> +static inline void iosf_mbi_punit_unlock(void) {} > >> + > >> #endif /* CONFIG_IOSF_MBI */ > >> > >> #endif /* IOSF_MBI_SYMS_H */ > >> diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c > >> index edf2c54..75d8135 100644 > >> --- a/arch/x86/platform/intel/iosf_mbi.c > >> +++ b/arch/x86/platform/intel/iosf_mbi.c > >> @@ -34,6 +34,7 @@ > >> > >> static struct pci_dev *mbi_pdev; > >> static DEFINE_SPINLOCK(iosf_mbi_lock); > >> +static DEFINE_MUTEX(iosf_mbi_punit_mutex); > >> > >> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset) > >> { > >> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void) > >> } > >> EXPORT_SYMBOL(iosf_mbi_available); > >> > >> +void iosf_mbi_punit_lock(void) > >> +{ > >> + mutex_lock(&iosf_mbi_punit_mutex); > >> +} > >> +EXPORT_SYMBOL(iosf_mbi_punit_lock); > >> + > >> +void iosf_mbi_punit_unlock(void) > >> +{ > >> + mutex_unlock(&iosf_mbi_punit_mutex); > >> +} > >> +EXPORT_SYMBOL(iosf_mbi_punit_unlock); > >> + > >> #ifdef CONFIG_IOSF_MBI_DEBUG > >> static u32 dbg_mdr; > >> static u32 dbg_mcr; > >> -- > >> 2.9.3 > > -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 9:26 ` Ville Syrjälä @ 2017-01-13 10:34 ` Jarkko Nikula 2017-01-13 10:51 ` Ville Syrjälä 2017-01-13 16:06 ` Hans de Goede 1 sibling, 1 reply; 25+ messages in thread From: Jarkko Nikula @ 2017-01-13 10:34 UTC (permalink / raw) To: Ville Syrjälä, Hans de Goede Cc: russianneuromancer @ ya . ru, intel-gfx, linux-i2c, Len Brown On 01/13/2017 11:26 AM, Ville Syrjälä wrote: > It also feels quite hand wavy since the punit could do whatever at > any time AFAIK. Eg. if there's some thermal event or something the > punit might kick into action. So trying to protect this from the OS > side might not be able to avoid these problems entirely. It feels like > there really should be some kind of shared hardware/firmware mutex > with the punit to arbitrate access to the i2c bus. > There is an HW semaphore for I2C access. It is implemented in drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans is adding support for Cherrytrail into it. -- Jarkko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 10:34 ` Jarkko Nikula @ 2017-01-13 10:51 ` Ville Syrjälä 2017-01-13 11:12 ` Jarkko Nikula 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-13 10:51 UTC (permalink / raw) To: Jarkko Nikula Cc: Hans de Goede, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote: > On 01/13/2017 11:26 AM, Ville Syrjälä wrote: > > It also feels quite hand wavy since the punit could do whatever at > > any time AFAIK. Eg. if there's some thermal event or something the > > punit might kick into action. So trying to protect this from the OS > > side might not be able to avoid these problems entirely. It feels like > > there really should be some kind of shared hardware/firmware mutex > > with the punit to arbitrate access to the i2c bus. > > > There is an HW semaphore for I2C access. It is implemented in > drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans > is adding support for Cherrytrail into it. Then why do we need anything else? -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 10:51 ` Ville Syrjälä @ 2017-01-13 11:12 ` Jarkko Nikula 2017-01-13 12:20 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Jarkko Nikula @ 2017-01-13 11:12 UTC (permalink / raw) To: Ville Syrjälä Cc: Hans de Goede, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx On 01/13/2017 12:51 PM, Ville Syrjälä wrote: > On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote: >> On 01/13/2017 11:26 AM, Ville Syrjälä wrote: >>> It also feels quite hand wavy since the punit could do whatever at >>> any time AFAIK. Eg. if there's some thermal event or something the >>> punit might kick into action. So trying to protect this from the OS >>> side might not be able to avoid these problems entirely. It feels like >>> there really should be some kind of shared hardware/firmware mutex >>> with the punit to arbitrate access to the i2c bus. >>> >> There is an HW semaphore for I2C access. It is implemented in >> drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans >> is adding support for Cherrytrail into it. > > Then why do we need anything else? > From this patch: "The punit on baytrail / cherrytrail systems is not only accessed through the iosf_mbi functions, but also by the i915 code." -- Jarkko ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 11:12 ` Jarkko Nikula @ 2017-01-13 12:20 ` Ville Syrjälä 0 siblings, 0 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-01-13 12:20 UTC (permalink / raw) To: Jarkko Nikula Cc: Hans de Goede, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx On Fri, Jan 13, 2017 at 01:12:15PM +0200, Jarkko Nikula wrote: > On 01/13/2017 12:51 PM, Ville Syrjälä wrote: > > On Fri, Jan 13, 2017 at 12:34:54PM +0200, Jarkko Nikula wrote: > >> On 01/13/2017 11:26 AM, Ville Syrjälä wrote: > >>> It also feels quite hand wavy since the punit could do whatever at > >>> any time AFAIK. Eg. if there's some thermal event or something the > >>> punit might kick into action. So trying to protect this from the OS > >>> side might not be able to avoid these problems entirely. It feels like > >>> there really should be some kind of shared hardware/firmware mutex > >>> with the punit to arbitrate access to the i2c bus. > >>> > >> There is an HW semaphore for I2C access. It is implemented in > >> drivers/i2c/busses/i2c-designware-baytrail.c and another set from Hans > >> is adding support for Cherrytrail into it. > > > > Then why do we need anything else? > > > From this patch: "The punit on baytrail / cherrytrail systems is not > only accessed through the iosf_mbi functions, but also by the i915 code." I don't see how that's relevant at all. Multiple things accessing the punit concurrently should be perfectly fine as long as they don't frob the same registers. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 9:26 ` Ville Syrjälä 2017-01-13 10:34 ` Jarkko Nikula @ 2017-01-13 16:06 ` Hans de Goede 2017-01-13 16:30 ` Ville Syrjälä 1 sibling, 1 reply; 25+ messages in thread From: Hans de Goede @ 2017-01-13 16:06 UTC (permalink / raw) To: Ville Syrjälä Cc: russianneuromancer @ ya . ru, intel-gfx, Jarkko Nikula, linux-i2c, Len Brown Hi, On 01/13/2017 10:26 AM, Ville Syrjälä wrote: > On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote: >> Hi, >> >> On 02-01-17 15:12, Ville Syrjälä wrote: >>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: >>>> The punit on baytrail / cherrytrail systems is not only accessed through >>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect >>>> the punit against simultaneous accesses and 2 functions to lock / unlock >>>> this mutex. >>> >>> I'm not sure which part of punit you're actually trying to protect >>> here. Some specific registers? >> >> The theory I'm going by is that for certain actions / certain requests >> we send to the punit, the punit needs to access the (axp288) pmic, to >> change (or enable / disable) certain voltages. > > At least for cpu/display/gt voltages that shouldn't really be the case. > The vcc/vnn/vgg rails are controlled via svid, not i2c. Are you sure? The ax288 pmic does not have a svid interface, only an i2c interface, and AFAICT its buck DCDC converters are used to feed all of these. > It also feels quite hand wavy since the punit could do whatever at > any time AFAIK. Eg. if there's some thermal event or something the > punit might kick into action. So trying to protect this from the OS > side might not be able to avoid these problems entirely. It feels like > there really should be some kind of shared hardware/firmware mutex > with the punit to arbitrate access to the i2c bus. Right, and there is such a mutex (which only gets used on systems with an axp288 pmic...) and we are taking this mutex before starting an i2c transaction on the pmic i2c bus. But this does not seem to be enough. It seems the the punit does not check the mutex before certain OS / host triggered actions. I guess it expects the host to do this itself. Please see my new (non RFC) version of this series I've posted. There are at least 2 problems when relying solely on the punit pmic i2c bus sempaphore: 1) CPU C1 <-> C6 transations happening while the pmic i2c bus is being accessed by the host cause the system to hang 2) i915 (runtime) suspend resume fails every other attempt with timeouts when trying to get a forcewake lock inn i915, often followed by a system freeze shortly after this. My non RFC version of this patch-set fixes both. So summarizing yes you are right that there should be some hardware mutex (there is, and we are already taking it), but unfortunately that does not seem to be enough, when explicitly requesting some power state transation, while another driver is acccessing the pmic i2c bus bad things happen. Adding some exclusion mechanism here seems to be necessary. Note that the i2c acccess vs i915 forcewake issues was first reported by an user who was trying my patches for fixing pmic i2c access on cht (fixing the semaphore code on cht) and then noticed a problem with the i915 driver as soon as the pmic i2c bus was used. He has also confirmed that the new non RFC version of my patches fix this. Regards, Hans _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 16:06 ` Hans de Goede @ 2017-01-13 16:30 ` Ville Syrjälä 2017-01-15 11:10 ` Hans de Goede 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-13 16:30 UTC (permalink / raw) To: Hans de Goede Cc: Jarkko Nikula, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx On Fri, Jan 13, 2017 at 05:06:52PM +0100, Hans de Goede wrote: > Hi, > > On 01/13/2017 10:26 AM, Ville Syrjälä wrote: > > On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote: > >> Hi, > >> > >> On 02-01-17 15:12, Ville Syrjälä wrote: > >>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: > >>>> The punit on baytrail / cherrytrail systems is not only accessed through > >>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect > >>>> the punit against simultaneous accesses and 2 functions to lock / unlock > >>>> this mutex. > >>> > >>> I'm not sure which part of punit you're actually trying to protect > >>> here. Some specific registers? > >> > >> The theory I'm going by is that for certain actions / certain requests > >> we send to the punit, the punit needs to access the (axp288) pmic, to > >> change (or enable / disable) certain voltages. > > > > At least for cpu/display/gt voltages that shouldn't really be the case. > > The vcc/vnn/vgg rails are controlled via svid, not i2c. > > Are you sure? The ax288 pmic does not have a svid interface, only > an i2c interface, and AFAICT its buck DCDC converters are used to > feed all of these. Yes, looks like you're right. I guess someone didn't want to spend three pins for svid. > > > It also feels quite hand wavy since the punit could do whatever at > > any time AFAIK. Eg. if there's some thermal event or something the > > punit might kick into action. So trying to protect this from the OS > > side might not be able to avoid these problems entirely. It feels like > > there really should be some kind of shared hardware/firmware mutex > > with the punit to arbitrate access to the i2c bus. > > Right, and there is such a mutex (which only gets used on systems > with an axp288 pmic...) and we are taking this mutex before starting > an i2c transaction on the pmic i2c bus. But this does not seem to be > enough. It seems the the punit does not check the mutex before > certain OS / host triggered actions. I guess it expects the host to > do this itself. > > Please see my new (non RFC) version of this series I've posted. > > There are at least 2 problems when relying solely on the punit > pmic i2c bus sempaphore: > > 1) CPU C1 <-> C6 transations happening while the pmic i2c bus > is being accessed by the host cause the system to hang > 2) i915 (runtime) suspend resume fails every other attempt > with timeouts when trying to get a forcewake lock inn i915, > often followed by a system freeze shortly after this. Hmm. But forcewake works at other times? That seems quite strange. Runtime suspend itself shouldn't really do much, and if we're still poking at the the hw then we haven't really even suspended anything yet, so having failing forcewake doesn't sounds at all good. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access 2017-01-13 16:30 ` Ville Syrjälä @ 2017-01-15 11:10 ` Hans de Goede 0 siblings, 0 replies; 25+ messages in thread From: Hans de Goede @ 2017-01-15 11:10 UTC (permalink / raw) To: Ville Syrjälä Cc: Jarkko Nikula, Len Brown, Jani Nikula, russianneuromancer @ ya . ru, linux-i2c, intel-gfx, tagorereddy Hi, On 13-01-17 17:30, Ville Syrjälä wrote: > On Fri, Jan 13, 2017 at 05:06:52PM +0100, Hans de Goede wrote: >> Hi, >> >> On 01/13/2017 10:26 AM, Ville Syrjälä wrote: >>> On Mon, Jan 02, 2017 at 03:21:13PM +0100, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 02-01-17 15:12, Ville Syrjälä wrote: >>>>> On Sun, Jan 01, 2017 at 09:14:00PM +0100, Hans de Goede wrote: >>>>>> The punit on baytrail / cherrytrail systems is not only accessed through >>>>>> the iosf_mbi functions, but also by the i915 code. Add a mutex to protect >>>>>> the punit against simultaneous accesses and 2 functions to lock / unlock >>>>>> this mutex. >>>>> >>>>> I'm not sure which part of punit you're actually trying to protect >>>>> here. Some specific registers? >>>> >>>> The theory I'm going by is that for certain actions / certain requests >>>> we send to the punit, the punit needs to access the (axp288) pmic, to >>>> change (or enable / disable) certain voltages. >>> >>> At least for cpu/display/gt voltages that shouldn't really be the case. >>> The vcc/vnn/vgg rails are controlled via svid, not i2c. >> >> Are you sure? The ax288 pmic does not have a svid interface, only >> an i2c interface, and AFAICT its buck DCDC converters are used to >> feed all of these. > > Yes, looks like you're right. I guess someone didn't want to spend three > pins for svid. > >> >>> It also feels quite hand wavy since the punit could do whatever at >>> any time AFAIK. Eg. if there's some thermal event or something the >>> punit might kick into action. So trying to protect this from the OS >>> side might not be able to avoid these problems entirely. It feels like >>> there really should be some kind of shared hardware/firmware mutex >>> with the punit to arbitrate access to the i2c bus. >> >> Right, and there is such a mutex (which only gets used on systems >> with an axp288 pmic...) and we are taking this mutex before starting >> an i2c transaction on the pmic i2c bus. But this does not seem to be >> enough. It seems the the punit does not check the mutex before >> certain OS / host triggered actions. I guess it expects the host to >> do this itself. >> >> Please see my new (non RFC) version of this series I've posted. >> >> There are at least 2 problems when relying solely on the punit >> pmic i2c bus sempaphore: >> >> 1) CPU C1 <-> C6 transations happening while the pmic i2c bus >> is being accessed by the host cause the system to hang >> 2) i915 (runtime) suspend resume fails every other attempt >> with timeouts when trying to get a forcewake lock inn i915, >> often followed by a system freeze shortly after this. > > Hmm. But forcewake works at other times? It depends on the workload, I believe the forcewake timeouts are caused by e.g. the axp288 fuel-gauge driver directly accessing the pmic i2c bus at the same time as the i915 driver is doing a forcewake. So in essence this is race and as such not 100% reproducible. With my workload (Fedora 25 with gnome3) full suspend + resume is a good way to reproduce. The bug reporter (tagorereddy) in: https://bugzilla.kernel.org/show_bug.cgi?id=155241 Is seeing this during normal use when using a kde / plasma desktop. Some history, this problem started surfacing when I fixed the i2c punit semaphore code in i2c-designware-baytrail.c to actually work on cht, before that systems with an axp288 any attempt to access the i2c bus by e.g. the axp288_fuel_gauge driver would result in -ETIMEOUT as the code would fail to acquire the punit i2c bus semaphore, this i2c-designware-baytrail.c cht bug has so far protected users against the described race (*). tagorereddy then tried my patches to get battery monitoring working on his cht device. Then he reported back in the above bug that he was getting forcewake timeouts + system hangs. I only noticed I could reproduce them myself on resume later (which was quite useful in actually developing the proposed fix). > That seems quite strange. > Runtime suspend itself shouldn't really do much, and if we're still > poking at the the hw then we haven't really even suspended anything > yet, so having failing forcewake doesn't sounds at all good. Sorry, I'm actually seeing these on a (full not runtime) resume, not suspend, it seems that at resume my setup has the ideal circumstances to hit the race. Regards, Hans *) Note as described in the cover letter of the non RFC version of this patch-set: https://www.spinics.net/lists/dri-devel/msg128896.html Disabling access to the pmic i2c bus (as the fixed bug does) is not a workable solution: "Unfortunately that will cause some major issues on affected devices: -No battery monitoring -No "AC" plugged in monitoring -If booted with a normal USB-A -> micro-USB cable, or no cable, plugged in and then the user replaces the cable with an otg USB-host cable / adapter, the id-pin shorting will enable a 5v boost convertor, but we need to disable the pmic's USB-Vbus path otherwise it will start drawing current from the boost convertor, leading to aprox 300mA of extra battery drain, this is done by the axp288_charger driver, which needs direct i2c access to the pmic bus" ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 2/4] i2c: designware-baytrail: Take punit lock on bus acquire 2017-01-01 20:13 [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses Hans de Goede 2017-01-01 20:14 ` [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access Hans de Goede @ 2017-01-01 20:14 ` Hans de Goede 2017-01-01 20:14 ` [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA Hans de Goede 2017-01-01 20:14 ` [RFC 4/4] drm/i915: valleyview: Take punit lock when modifying punit settings Hans de Goede 3 siblings, 0 replies; 25+ messages in thread From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw) To: Jarkko Nikula, Len Brown Cc: Jani Nikula, Ville Syrjälä, russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede Take the punit lock to stop others from accessing the punit while the pmic i2c bus is in use. This is necessary because accessing the punit from the kernel may result in the punit trying to access the pmic i2c bus, which results in a hang when it happens while we own the pmic i2c bus semaphore. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c index 3effc9a..507875d 100644 --- a/drivers/i2c/busses/i2c-designware-baytrail.c +++ b/drivers/i2c/busses/i2c-designware-baytrail.c @@ -61,6 +61,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev) 0, PUNIT_SEMAPHORE_BIT)) dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n"); + iosf_mbi_punit_unlock(); + pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE); } @@ -86,6 +88,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev) */ pm_qos_update_request(&dev->pm_qos, 0); + iosf_mbi_punit_lock(); + /* host driver writes to side band semaphore register */ ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, sem); if (ret) { -- 2.9.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-01 20:13 [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses Hans de Goede 2017-01-01 20:14 ` [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access Hans de Goede 2017-01-01 20:14 ` [RFC 2/4] i2c: designware-baytrail: Take punit lock on bus acquire Hans de Goede @ 2017-01-01 20:14 ` Hans de Goede 2017-01-01 20:24 ` [Intel-gfx] " Chris Wilson 2017-01-01 20:14 ` [RFC 4/4] drm/i915: valleyview: Take punit lock when modifying punit settings Hans de Goede 3 siblings, 1 reply; 25+ messages in thread From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw) To: Jarkko Nikula, Len Brown Cc: Jani Nikula, Ville Syrjälä, russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except for intel_set_rps(). Since intel_set_rps can for example be called from sysfs store functions, there is no guarantee this is already done, so add an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4b12637..cc4fbd7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) { - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + /* Wake up the media well, as that takes a lot less + * power than the Render well. + */ + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); valleyview_set_rps(dev_priv, val); - else + intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); + } else gen6_set_rps(dev_priv, val); } -- 2.9.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-01 20:14 ` [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA Hans de Goede @ 2017-01-01 20:24 ` Chris Wilson 2017-01-01 20:48 ` Hans de Goede 0 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2017-01-01 20:24 UTC (permalink / raw) To: Hans de Goede Cc: Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > for intel_set_rps(). Since intel_set_rps can for example be called from > sysfs store functions, there is no guarantee this is already done, so add > an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 4b12637..cc4fbd7 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > { > - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > + /* Wake up the media well, as that takes a lot less > + * power than the Render well. > + */ > + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > valleyview_set_rps(dev_priv, val); Both powerwells are woken for rps. (Taking one but not the other has no benefit, and very misleading.) The forcewake is already held by the lower level routines, taking the wakelock in the caller is an optimisation that is only interesting if there is a danger from the forcewake being dropped mid-sequence (due to preemption whatever). -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-01 20:24 ` [Intel-gfx] " Chris Wilson @ 2017-01-01 20:48 ` Hans de Goede 2017-01-02 11:37 ` [Intel-gfx] " Chris Wilson 0 siblings, 1 reply; 25+ messages in thread From: Hans de Goede @ 2017-01-01 20:48 UTC (permalink / raw) To: Chris Wilson, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c Hi, On 01-01-17 21:24, Chris Wilson wrote: > On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: >> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except >> for intel_set_rps(). Since intel_set_rps can for example be called from >> sysfs store functions, there is no guarantee this is already done, so add >> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >> index 4b12637..cc4fbd7 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, >> >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) >> { >> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) >> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { >> + /* Wake up the media well, as that takes a lot less >> + * power than the Render well. >> + */ >> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); >> valleyview_set_rps(dev_priv, val); > > Both powerwells are woken for rps. (Taking one but not the other has no > benefit, and very misleading.) The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the existing code in vlv_set_rps_idle(). > The forcewake is already held by the > lower level routines, taking the wakelock in the caller is an optimisation > that is only interesting if there is a danger from the forcewake being > dropped mid-sequence (due to preemption whatever). We're also accessing the punit in valleyview_set_rps() and I've seen several patches (in the android x86 kernel) suggesting that we need to take a wakelock while doing this. Regards, Hans _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-01 20:48 ` Hans de Goede @ 2017-01-02 11:37 ` Chris Wilson 2017-01-02 12:40 ` Hans de Goede 2017-01-02 14:10 ` Ville Syrjälä 0 siblings, 2 replies; 25+ messages in thread From: Chris Wilson @ 2017-01-02 11:37 UTC (permalink / raw) To: Hans de Goede Cc: Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > Hi, > > On 01-01-17 21:24, Chris Wilson wrote: > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > >>for intel_set_rps(). Since intel_set_rps can for example be called from > >>sysfs store functions, there is no guarantee this is already done, so add > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > >> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>--- > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>index 4b12637..cc4fbd7 100644 > >>--- a/drivers/gpu/drm/i915/intel_pm.c > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > >> > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > >> { > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > >>+ /* Wake up the media well, as that takes a lot less > >>+ * power than the Render well. > >>+ */ > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > >> valleyview_set_rps(dev_priv, val); > > > >Both powerwells are woken for rps. (Taking one but not the other has no > >benefit, and very misleading.) > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > existing code in vlv_set_rps_idle(). It's not correct there either... > > The forcewake is already held by the > >lower level routines, taking the wakelock in the caller is an optimisation > >that is only interesting if there is a danger from the forcewake being > >dropped mid-sequence (due to preemption whatever). > > We're also accessing the punit in valleyview_set_rps() and I've seen several > patches (in the android x86 kernel) suggesting that we need to take a wakelock > while doing this. It is quite likely that we do have to guarantee to keep the punit awake during long sequences. That would be an acceptable rationale (but not in the caller!). -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 11:37 ` [Intel-gfx] " Chris Wilson @ 2017-01-02 12:40 ` Hans de Goede 2017-01-02 14:10 ` Ville Syrjälä 1 sibling, 0 replies; 25+ messages in thread From: Hans de Goede @ 2017-01-02 12:40 UTC (permalink / raw) To: Chris Wilson, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c Hi, On 02-01-17 12:37, Chris Wilson wrote: > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: >> Hi, >> >> On 01-01-17 21:24, Chris Wilson wrote: >>> On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: >>>> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except >>>> for intel_set_rps(). Since intel_set_rps can for example be called from >>>> sysfs store functions, there is no guarantee this is already done, so add >>>> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>> index 4b12637..cc4fbd7 100644 >>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, >>>> >>>> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) >>>> { >>>> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) >>>> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { >>>> + /* Wake up the media well, as that takes a lot less >>>> + * power than the Render well. >>>> + */ >>>> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); >>>> valleyview_set_rps(dev_priv, val); >>> >>> Both powerwells are woken for rps. (Taking one but not the other has no >>> benefit, and very misleading.) >> >> The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the >> existing code in vlv_set_rps_idle(). > > It's not correct there either... > >>> The forcewake is already held by the >>> lower level routines, taking the wakelock in the caller is an optimisation >>> that is only interesting if there is a danger from the forcewake being >>> dropped mid-sequence (due to preemption whatever). >> >> We're also accessing the punit in valleyview_set_rps() and I've seen several >> patches (in the android x86 kernel) suggesting that we need to take a wakelock >> while doing this. > > It is quite likely that we do have to guarantee to keep the punit awake > during long sequences. That would be an acceptable rationale (but not in > the caller!). So what would be a good approach, take FORCEWAKE_ALL in valleyview_set_rps() itself (and drop the existing code from vlv_set_rps_idle()) ? Regards, Hans ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 11:37 ` [Intel-gfx] " Chris Wilson 2017-01-02 12:40 ` Hans de Goede @ 2017-01-02 14:10 ` Ville Syrjälä 2017-01-02 14:21 ` Chris Wilson 1 sibling, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-02 14:10 UTC (permalink / raw) To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > Hi, > > > > On 01-01-17 21:24, Chris Wilson wrote: > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > >>sysfs store functions, there is no guarantee this is already done, so add > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > >> > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > >>--- > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > >> > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > >>index 4b12637..cc4fbd7 100644 > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > >> > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > >> { > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > >>+ /* Wake up the media well, as that takes a lot less > > >>+ * power than the Render well. > > >>+ */ > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > >> valleyview_set_rps(dev_priv, val); > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > >benefit, and very misleading.) > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > existing code in vlv_set_rps_idle(). > > It's not correct there either... Why not? If the render well is in rc6 already we don't want to waste power by waking it. The only reason we have to wake up *something* is so that the gpll frequency actually gets changed. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 14:10 ` Ville Syrjälä @ 2017-01-02 14:21 ` Chris Wilson 2017-01-02 14:40 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2017-01-02 14:21 UTC (permalink / raw) To: Ville Syrjälä Cc: Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote: > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 01-01-17 21:24, Chris Wilson wrote: > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > > >>sysfs store functions, there is no guarantee this is already done, so add > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > > >> > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > >>--- > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > > >> > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > >>index 4b12637..cc4fbd7 100644 > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > > >> > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > > >> { > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > > >>+ /* Wake up the media well, as that takes a lot less > > > >>+ * power than the Render well. > > > >>+ */ > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > > >> valleyview_set_rps(dev_priv, val); > > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > > >benefit, and very misleading.) > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > > existing code in vlv_set_rps_idle(). > > > > It's not correct there either... > > Why not? If the render well is in rc6 already we don't want to waste > power by waking it. The only reason we have to wake up *something* is > so that the gpll frequency actually gets changed. If the register write requires the powerwell, the mmio access will take the powerwell. Manually taking the wakelock has to be for a reason, the one given is bogus. (That is if the register write only requires meda access that is taken, but iirc, rps requires both, so both are taken inside.) If the claimed benefit is indeed real, and there is indeed freedom of choice, we want that inside the mmio accessor to choose the cheaper well. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 14:21 ` Chris Wilson @ 2017-01-02 14:40 ` Ville Syrjälä 2017-01-02 14:53 ` [Intel-gfx] " Chris Wilson 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-02 14:40 UTC (permalink / raw) To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote: > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote: > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 01-01-17 21:24, Chris Wilson wrote: > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > > > >>sysfs store functions, there is no guarantee this is already done, so add > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > > > >> > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > >>--- > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > > > >> > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > >>index 4b12637..cc4fbd7 100644 > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > > > >> > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > > > >> { > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > > > >>+ /* Wake up the media well, as that takes a lot less > > > > >>+ * power than the Render well. > > > > >>+ */ > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > > > >> valleyview_set_rps(dev_priv, val); > > > > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > > > >benefit, and very misleading.) > > > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > > > existing code in vlv_set_rps_idle(). > > > > > > It's not correct there either... > > > > Why not? If the render well is in rc6 already we don't want to waste > > power by waking it. The only reason we have to wake up *something* is > > so that the gpll frequency actually gets changed. > > If the register write requires the powerwell, the mmio access will take > the powerwell. The register write doesn't require a power well. It's a sideband access. The punit will simply not change the GPLL frequency if the GPLL is currently not running (which will/can happen when all power wells are asleep). That in itself doesn't sound too back (why change the frequency if the thing isn't even running, right?). But the real problem is that the punit will not let the voltage on the rail to drop either until the new frequency gets programmed into the GPLL. Hence if the GPU goes idle before we've dropped the GPLL frequency to the minimum value, we will waste power by having a needlessly high voltage. Originally we tried to avoid this problem via vlv_force_gfx_clock(), which should just force the GPLL to turn on without powering on any power wells. But that caused some spurious WARNs or something IIRC, so we had to come up with another workaround. And since powering either well is sufficient we chose to use the cheaper media well. > Manually taking the wakelock has to be for a reason, the > one given is bogus. (That is if the register write only requires meda > access that is taken, but iirc, rps requires both, so both are taken > inside.) If the claimed benefit is indeed real, and there is indeed > freedom of choice, we want that inside the mmio accessor to choose the > cheaper well. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 14:40 ` Ville Syrjälä @ 2017-01-02 14:53 ` Chris Wilson 2017-01-02 15:02 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Chris Wilson @ 2017-01-02 14:53 UTC (permalink / raw) To: Ville Syrjälä Cc: Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote: > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote: > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote: > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > > > > Hi, > > > > > > > > > > On 01-01-17 21:24, Chris Wilson wrote: > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > > > > >>sysfs store functions, there is no guarantee this is already done, so add > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > > > > >> > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > >>--- > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > >> > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > >>index 4b12637..cc4fbd7 100644 > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > > > > >> > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > > > > >> { > > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > > > > >>+ /* Wake up the media well, as that takes a lot less > > > > > >>+ * power than the Render well. > > > > > >>+ */ > > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > > > > >> valleyview_set_rps(dev_priv, val); > > > > > > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > > > > >benefit, and very misleading.) > > > > > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > > > > existing code in vlv_set_rps_idle(). > > > > > > > > It's not correct there either... > > > > > > Why not? If the render well is in rc6 already we don't want to waste > > > power by waking it. The only reason we have to wake up *something* is > > > so that the gpll frequency actually gets changed. > > > > If the register write requires the powerwell, the mmio access will take > > the powerwell. > > The register write doesn't require a power well. It's a sideband access. > The punit will simply not change the GPLL frequency if the GPLL is > currently not running (which will/can happen when all power wells are > asleep). That in itself doesn't sound too back (why change the > frequency if the thing isn't even running, right?). But the real problem > is that the punit will not let the voltage on the rail to drop > either until the new frequency gets programmed into the GPLL. Hence if > the GPU goes idle before we've dropped the GPLL frequency to the > minimum value, we will waste power by having a needlessly high voltage. > > Originally we tried to avoid this problem via vlv_force_gfx_clock(), > which should just force the GPLL to turn on without powering on > any power wells. But that caused some spurious WARNs or something > IIRC, so we had to come up with another workaround. And since powering > either well is sufficient we chose to use the cheaper media well. That explains set_idle() (and would be a better comment that the one there). But not set_rps(), since there we don't care if the write is delayed until the GPU is next active. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 14:53 ` [Intel-gfx] " Chris Wilson @ 2017-01-02 15:02 ` Ville Syrjälä 2017-01-02 15:08 ` Ville Syrjälä 0 siblings, 1 reply; 25+ messages in thread From: Ville Syrjälä @ 2017-01-02 15:02 UTC (permalink / raw) To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote: > On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote: > > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote: > > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote: > > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > > > > > Hi, > > > > > > > > > > > > On 01-01-17 21:24, Chris Wilson wrote: > > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > > > > > >>sysfs store functions, there is no guarantee this is already done, so add > > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > > > > > >> > > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > >>--- > > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > >> > > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > >>index 4b12637..cc4fbd7 100644 > > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > > > > > >> > > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > > > > > >> { > > > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > > > > > >>+ /* Wake up the media well, as that takes a lot less > > > > > > >>+ * power than the Render well. > > > > > > >>+ */ > > > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > > > > > >> valleyview_set_rps(dev_priv, val); > > > > > > > > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > > > > > >benefit, and very misleading.) > > > > > > > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > > > > > existing code in vlv_set_rps_idle(). > > > > > > > > > > It's not correct there either... > > > > > > > > Why not? If the render well is in rc6 already we don't want to waste > > > > power by waking it. The only reason we have to wake up *something* is > > > > so that the gpll frequency actually gets changed. > > > > > > If the register write requires the powerwell, the mmio access will take > > > the powerwell. > > > > The register write doesn't require a power well. It's a sideband access. > > The punit will simply not change the GPLL frequency if the GPLL is > > currently not running (which will/can happen when all power wells are > > asleep). That in itself doesn't sound too back (why change the > > frequency if the thing isn't even running, right?). But the real problem > > is that the punit will not let the voltage on the rail to drop > > either until the new frequency gets programmed into the GPLL. Hence if > > the GPU goes idle before we've dropped the GPLL frequency to the > > minimum value, we will waste power by having a needlessly high voltage. > > > > Originally we tried to avoid this problem via vlv_force_gfx_clock(), > > which should just force the GPLL to turn on without powering on > > any power wells. But that caused some spurious WARNs or something > > IIRC, so we had to come up with another workaround. And since powering > > either well is sufficient we chose to use the cheaper media well. > > That explains set_idle() (and would be a better comment that the one > there). But not set_rps(), since there we don't care if the write is > delayed until the GPU is next active. I don't see any forcewakes in set_rps() -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA 2017-01-02 15:02 ` Ville Syrjälä @ 2017-01-02 15:08 ` Ville Syrjälä 0 siblings, 0 replies; 25+ messages in thread From: Ville Syrjälä @ 2017-01-02 15:08 UTC (permalink / raw) To: Chris Wilson, Hans de Goede, Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx, linux-i2c On Mon, Jan 02, 2017 at 05:02:25PM +0200, Ville Syrjälä wrote: > On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote: > > On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote: > > > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote: > > > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote: > > > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote: > > > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On 01-01-17 21:24, Chris Wilson wrote: > > > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote: > > > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except > > > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from > > > > > > > >>sysfs store functions, there is no guarantee this is already done, so add > > > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps. > > > > > > > >> > > > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > > >>--- > > > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- > > > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > >> > > > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > >>index 4b12637..cc4fbd7 100644 > > > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, > > > > > > > >> > > > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) > > > > > > > >> { > > > > > > > >>- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > > > > > > > >>+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { > > > > > > > >>+ /* Wake up the media well, as that takes a lot less > > > > > > > >>+ * power than the Render well. > > > > > > > >>+ */ > > > > > > > >>+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); > > > > > > > >> valleyview_set_rps(dev_priv, val); > > > > > > > > > > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no > > > > > > > >benefit, and very misleading.) > > > > > > > > > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the > > > > > > > existing code in vlv_set_rps_idle(). > > > > > > > > > > > > It's not correct there either... > > > > > > > > > > Why not? If the render well is in rc6 already we don't want to waste > > > > > power by waking it. The only reason we have to wake up *something* is > > > > > so that the gpll frequency actually gets changed. > > > > > > > > If the register write requires the powerwell, the mmio access will take > > > > the powerwell. > > > > > > The register write doesn't require a power well. It's a sideband access. > > > The punit will simply not change the GPLL frequency if the GPLL is > > > currently not running (which will/can happen when all power wells are > > > asleep). That in itself doesn't sound too back (why change the > > > frequency if the thing isn't even running, right?). But the real problem > > > is that the punit will not let the voltage on the rail to drop > > > either until the new frequency gets programmed into the GPLL. Hence if > > > the GPU goes idle before we've dropped the GPLL frequency to the > > > minimum value, we will waste power by having a needlessly high voltage. > > > > > > Originally we tried to avoid this problem via vlv_force_gfx_clock(), > > > which should just force the GPLL to turn on without powering on > > > any power wells. But that caused some spurious WARNs or something > > > IIRC, so we had to come up with another workaround. And since powering > > > either well is sufficient we chose to use the cheaper media well. > > > > That explains set_idle() (and would be a better comment that the one > > there). But not set_rps(), since there we don't care if the write is > > delayed until the GPU is next active. > > I don't see any forcewakes in set_rps() Ah, it was this patch which wants to add that. And indeed that doesn't make sense. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC 4/4] drm/i915: valleyview: Take punit lock when modifying punit settings 2017-01-01 20:13 [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses Hans de Goede ` (2 preceding siblings ...) 2017-01-01 20:14 ` [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA Hans de Goede @ 2017-01-01 20:14 ` Hans de Goede 3 siblings, 0 replies; 25+ messages in thread From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw) To: Jarkko Nikula, Len Brown Cc: Jani Nikula, Ville Syrjälä, russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede Make sure the punit i2c bus is not in use when we send a request to the punit by calling iosf_mbi_punit_lock() / iosf_mbi_punit_unlock() around punit write accesses. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/Kconfig | 1 + drivers/gpu/drm/i915/intel_display.c | 6 ++++++ drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++++ drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++ 4 files changed, 37 insertions(+) diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig index 5ddde73..0fe7443 100644 --- a/drivers/gpu/drm/i915/Kconfig +++ b/drivers/gpu/drm/i915/Kconfig @@ -4,6 +4,7 @@ config DRM_I915 depends on X86 && PCI select INTEL_GTT select INTERVAL_TREE + select IOSF_MBI # we need shmfs for the swappable backing store, and in particular # the shmem_readpage() which depends upon tmpfs select SHMEM diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fec8eb3..b8be6ea 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -47,6 +47,7 @@ #include <drm/drm_rect.h> #include <linux/dma_remapping.h> #include <linux/reservation.h> +#include <asm/iosf_mbi.h> static bool is_mmio_work(struct intel_flip_work *work) { @@ -6423,6 +6424,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) cmd = 0; mutex_lock(&dev_priv->rps.hw_lock); + iosf_mbi_punit_lock(); + val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); val &= ~DSPFREQGUAR_MASK; val |= (cmd << DSPFREQGUAR_SHIFT); @@ -6432,6 +6435,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk) 50)) { DRM_ERROR("timed out waiting for CDclk change\n"); } + iosf_mbi_punit_unlock(); mutex_unlock(&dev_priv->rps.hw_lock); mutex_lock(&dev_priv->sb_lock); @@ -6499,6 +6503,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk) cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1; mutex_lock(&dev_priv->rps.hw_lock); + iosf_mbi_punit_lock(); val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); val &= ~DSPFREQGUAR_MASK_CHV; val |= (cmd << DSPFREQGUAR_SHIFT_CHV); @@ -6508,6 +6513,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk) 50)) { DRM_ERROR("timed out waiting for CDclk change\n"); } + iosf_mbi_punit_unlock(); mutex_unlock(&dev_priv->rps.hw_lock); intel_update_cdclk(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index cc4fbd7..31f88f1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -32,6 +32,7 @@ #include "../../../platform/x86/intel_ips.h" #include <linux/module.h> #include <drm/drm_atomic_helper.h> +#include <asm/iosf_mbi.h> /** * DOC: RC6 @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable) u32 val; mutex_lock(&dev_priv->rps.hw_lock); + iosf_mbi_punit_lock(); val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2); if (enable) @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable) FORCE_DDR_FREQ_REQ_ACK) == 0, 3)) DRM_ERROR("timed out waiting for Punit DDR DVFS request\n"); + iosf_mbi_punit_unlock(); mutex_unlock(&dev_priv->rps.hw_lock); } @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable) u32 val; mutex_lock(&dev_priv->rps.hw_lock); + iosf_mbi_punit_lock(); val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); if (enable) @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable) val &= ~DSP_MAXFIFO_PM5_ENABLE; vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val); + iosf_mbi_punit_unlock(); mutex_unlock(&dev_priv->rps.hw_lock); } @@ -4546,6 +4551,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev) if (IS_CHERRYVIEW(dev_priv)) { mutex_lock(&dev_priv->rps.hw_lock); + iosf_mbi_punit_lock(); val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); if (val & DSP_MAXFIFO_PM5_ENABLE) @@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev) wm->level = VLV_WM_LEVEL_DDR_DVFS; } + iosf_mbi_punit_unlock(); mutex_unlock(&dev_priv->rps.hw_lock); } @@ -5004,11 +5011,15 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv) if (dev_priv->rps.cur_freq <= val) return; + iosf_mbi_punit_lock(); + /* Wake up the media well, as that takes a lot less * power than the Render well. */ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); valleyview_set_rps(dev_priv, val); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); + + iosf_mbi_punit_unlock(); } void gen6_rps_busy(struct drm_i915_private *dev_priv) @@ -5097,12 +5108,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv, void intel_set_rps(struct drm_i915_private *dev_priv, u8 val) { if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) { + iosf_mbi_punit_lock(); /* Wake up the media well, as that takes a lot less * power than the Render well. */ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA); valleyview_set_rps(dev_priv, val); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA); + iosf_mbi_punit_unlock(); } else gen6_set_rps(dev_priv, val); } @@ -5999,6 +6012,8 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv) cherryview_check_pctx(dev_priv); + iosf_mbi_punit_lock(); + /* 1a & 1b: Get forcewake during program sequence. Although the driver * hasn't enabled a state yet where we need forcewake, BIOS may have.*/ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); @@ -6068,6 +6083,8 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv) reset_rps(dev_priv, valleyview_set_rps); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + + iosf_mbi_punit_unlock(); } static void valleyview_enable_rps(struct drm_i915_private *dev_priv) @@ -6087,6 +6104,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv) I915_WRITE(GTFIFODBG, gtfifodbg); } + iosf_mbi_punit_lock(); + /* If VLV, Forcewake all wells, else re-direct to regular path */ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); @@ -6149,6 +6168,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv) reset_rps(dev_priv, valleyview_set_rps); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); + + iosf_mbi_punit_unlock(); } static unsigned long intel_pxfreq(u32 vidfreq) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index c0b7e95..17922ae 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -28,6 +28,7 @@ #include <linux/pm_runtime.h> #include <linux/vgaarb.h> +#include <asm/iosf_mbi.h> #include "i915_drv.h" #include "intel_drv.h" @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv, if (COND) goto out; + iosf_mbi_punit_lock(); + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL); ctrl &= ~mask; ctrl |= state; @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv, state, vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL)); + iosf_mbi_punit_unlock(); + #undef COND out: @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv, if (COND) goto out; + iosf_mbi_punit_lock(); + ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ); ctrl &= ~DP_SSC_MASK(pipe); ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe); @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv, state, vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ)); + iosf_mbi_punit_unlock(); + #undef COND out: -- 2.9.3 ^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2017-01-15 11:10 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-01 20:13 [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses Hans de Goede 2017-01-01 20:14 ` [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access Hans de Goede 2017-01-02 14:12 ` Ville Syrjälä 2017-01-02 14:21 ` Hans de Goede 2017-01-13 9:26 ` Ville Syrjälä 2017-01-13 10:34 ` Jarkko Nikula 2017-01-13 10:51 ` Ville Syrjälä 2017-01-13 11:12 ` Jarkko Nikula 2017-01-13 12:20 ` Ville Syrjälä 2017-01-13 16:06 ` Hans de Goede 2017-01-13 16:30 ` Ville Syrjälä 2017-01-15 11:10 ` Hans de Goede 2017-01-01 20:14 ` [RFC 2/4] i2c: designware-baytrail: Take punit lock on bus acquire Hans de Goede 2017-01-01 20:14 ` [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA Hans de Goede 2017-01-01 20:24 ` [Intel-gfx] " Chris Wilson 2017-01-01 20:48 ` Hans de Goede 2017-01-02 11:37 ` [Intel-gfx] " Chris Wilson 2017-01-02 12:40 ` Hans de Goede 2017-01-02 14:10 ` Ville Syrjälä 2017-01-02 14:21 ` Chris Wilson 2017-01-02 14:40 ` Ville Syrjälä 2017-01-02 14:53 ` [Intel-gfx] " Chris Wilson 2017-01-02 15:02 ` Ville Syrjälä 2017-01-02 15:08 ` Ville Syrjälä 2017-01-01 20:14 ` [RFC 4/4] drm/i915: valleyview: Take punit lock when modifying punit settings Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.