Enable gpu switching on the MacBook Pro
Subsystems (10)
v2 cover Enable gpu switching on the MacBook Pro
This is a follow-up to the v1 posted in April:
http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
Patches 1 - 17 enable GPU switching on the pre-retina MacBook Pro. These were tested successfully by multiple people and solve two tickets in Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=88861
https://bugs.freedesktop.org/show_bug.cgi?id=61115
Patches 18 - 22 are a preview of how we're tackling retina support. Those are marked experimental and are NOT ready to be merged yet. Feedback on them is welcome.
The patches are based on drm-next.
They were tested on the following hardware (thanks a lot everyone!):
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
What's new:
* By default the MBP boots with the display switched to the discrete GPU but it can be forced to the integrated GPU with an EFI boot variable. Here's a handy tool for that: https://github.com/0xbb/gpu-switch
v1 didn't work in this configuration, v2 does.
* Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback. v2 simply generates a hotplug event instead. nouveau polls its outputs every 10 seconds so we want it to poll immediately once apple-gmux registers. That is achieved by the hotplug event. The i915 driver is changed to behave identically to nouveau. (Right now it deletes LVDS and eDP connectors from the mode configuration if they can't be probed, deeming them to be ghosts.)
* Framebuffer recreation if the inactive GPU initializes before the apple-gmux module (i.e. discarding the default 1024x768 fb and replacing with a new one with the actual panel resolution): v1 only supported this for i915, v2 has a generic solution which works with nouveau and radeon as well. (Necessary if the discrete GPU is forced to be the inactive one on boot via the EFI variable.)
* Generally lots of rough edges were smoothed to hopefully make the patches more suitable for merging. E.g. there's a bug in i915 where the SSC status set by BIOS is preserved too late and v1 only contained a workaround, whereas v2 contains a proper fix in a separate commit.
The long journey towards retina support:
The pixel clock required for retina resolution is not supported by LVDS (which was used on pre-retinas), necessitating eDP instead. Problem is, the gmux register which switches the DDC lines on pre-retinas doesn't switch the AUX channel on retinas. Disassembling the OS X driver revealed that the gmux in retina MBPs gained an additional register 0x7f which gets written to when setting up the eDP configuration. There was some hope that this might switch the AUX channel. Alas, we tried writing various values to that register but were unable to get the inactive GPU to talk to the panel. The purpose of register 0x7f remains a mystery.
Teardowns of the first generation retina MBP name the NXP CBTL06142 and TI HD3SS212 as multiplexers and according to the data sheets I've found, neither supports switching the AUX channel separately from the main link.
Matthew Garrett had the idea of having the active GPU stash the EDID and the first 8 bytes of the DPCD (receiver capabilities) and letting the inactive GPU retrieve that data. I rebased and rewrote his patches and got everything working, only to discover that the drivers are unhappy with just 8 bytes of DPCD. They need full access to the DPCD to set up their outputs. We could stash the entire DPCD but some parts of it are mutable so the stashed data may become stale when the active GPU performs writes to the DPCD.
So I had the idea of using the active GPU as a proxy to talk to the panel, thus emulating switching of the AUX channel in software. We can leverage the struct drm_dp_aux and i2c_adapter (for DDC) to achieve this, swapping the inactive GPU's structs with those of the active GPU on the fly. That approach is implemented in patches 18 - 22 but there are still some driver issues that I'm debugging. The current status as per the latest logs Bruno sent me is that i915 rejects the mode retrieved via proxying with CLOCK_HIGH and nouveau aborts link training halfway through. Bottom line is that it's not yet working but we're getting closer.
As a side effect, the pre-retinas gain a second way to initialize their
outputs: They can either use gmux to switch the DDC lines, or use the active GPU as a proxy for the DDC communication. Which method gets used depends on the order in which the drivers initialize, the inactive GPU will happily use whatever is available and it automatically receives a hotplug event once either method becomes ready for use.
But, once again, the patches implementing proxying (patches 18 - 22) are still in a state of flux and not ready for prime time, unlike the prior ones which seem stable. Folks are hereby invited to poke holes into them and I'm looking forward to your feedback.
Thanks,
Lukas
Dave Airlie (1):
vga_switcheroo: Lock/unlock DDC lines
Lukas Wunner (15):
vga_switcheroo: Lock/unlock DDC lines harder
Revert "vga_switcheroo: Add helper function to get the active client" Revert "vga_switcheroo: add reprobe hook for fbcon to recheck
connected outputs."
drm/nouveau: Lock/unlock DDC lines on probe
vga_switcheroo: Generate hotplug event on handler and proxy
registration
drm/i915: Preserve SSC earlier
drm/i915: Reprobe eDP and LVDS connectors on hotplug event drm/i915: On fb alloc failure, unref gem object where it gets refed
drm: Create new fb and replace default 1024x768 fb on hotplug event drm/nouveau/timer: Fall back to kernel timer if GPU timer read failed
vga_switcheroo: Allow using active client as proxy when reading
DDC/AUX
drm: Amend struct drm_dp_aux with connector attribute
drm: Use vga_switcheroo active client as proxy when reading DDC/AUX drm/nouveau/i2c: Use vga_switcheroo active client as proxy when
reading DDC/AUX
drm/nouveau: Use vga_switcheroo active client as proxy when probing
DDC on LVDS
Matthew Garrett (1):
apple-gmux: Assign apple_gmux_data before registering
Seth Forshee (4):
vga_switcheroo: Add support for switching only the DDC
vga_switcheroo: Add helper function to get the active client
apple-gmux: Add switch_ddc support
drm/edid: Switch DDC when reading the EDID
Tvrtko Ursulin (1):
drm/i915: Fix failure paths around initial fbdev allocation
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 - drivers/gpu/drm/drm_dp_helper.c | 14 ++ drivers/gpu/drm/drm_edid.c | 23 ++- drivers/gpu/drm/drm_fb_helper.c | 41 ++++- drivers/gpu/drm/i915/i915_dma.c | 1 - drivers/gpu/drm/i915/intel_display.c | 91 +++++++--- drivers/gpu/drm/i915/intel_dp.c | 39 +++-- drivers/gpu/drm/i915/intel_drv.h | 6 + drivers/gpu/drm/i915/intel_fbdev.c | 46 ++--- drivers/gpu/drm/i915/intel_lvds.c | 67 ++++--- drivers/gpu/drm/i915/intel_panel.c | 4 +- drivers/gpu/drm/msm/edp/edp_connector.c | 2 + drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h | 1 + drivers/gpu/drm/nouveau/nouveau_connector.c | 18 +- drivers/gpu/drm/nouveau/nouveau_dp.c | 20 +++ drivers/gpu/drm/nouveau/nouveau_fbcon.c | 10 +- drivers/gpu/drm/nouveau/nouveau_vga.c | 8 - drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c | 6 +- drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c | 2 +- drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h | 1 + drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 24 +++ drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c | 4 + drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c | 9 + drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h | 1 + drivers/gpu/drm/radeon/atombios_dp.c | 1 + drivers/gpu/drm/radeon/radeon_device.c | 1 - drivers/gpu/drm/radeon/radeon_fb.c | 11 +- drivers/gpu/drm/tegra/sor.c | 1 + drivers/gpu/vga/vga_switcheroo.c | 204 +++++++++++++++++++++- drivers/platform/x86/apple-gmux.c | 35 +++- include/drm/drm_dp_helper.h | 5 + include/linux/vga_switcheroo.h | 18 +- 32 files changed, 590 insertions(+), 125 deletions(-)
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
Lukas Wunner
reviewerPATCH v2 1/22 vga_switcheroo: Add support for switching only the DDC
From: Seth Forshee <[email protected]>
During graphics driver initialization its useful to be able to mux only the DDC to the inactive client in order to read the EDID. Add a switch_ddc callback to allow capable handlers to provide this functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux only the DDC.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/vga/vga_switcheroo.c | 39 ++++++++++++++++++++++++++++++++++++++- include/linux/vga_switcheroo.h | 4 ++++ 2 files changed, 42 insertions(+), 1 deletion(-)
drivers/gpu/vga/vga_switcheroo.c +38 -1
@@ -256,6 +256,29 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev, }EXPORT_SYMBOL (vga_switcheroo_client_fb_set); +int vga_switcheroo_switch_ddc (struct pci_dev *pdev) +{ +int ret =0 ; +int id; + +mutex_lock (&vgasr_mutex); + +if (!vgasr_priv.handler) { + ret = -ENODEV ; + goto out; + } + +if (vgasr_priv.handler->switch_ddc) { + id = vgasr_priv.handler->get_client_id (pdev); + ret = vgasr_priv.handler->switch_ddc (id); + } + +out: +mutex_unlock (&vgasr_mutex); +return ret; +} +EXPORT_SYMBOL (vga_switcheroo_switch_ddc); +static int vga_switcheroo_show (struct seq_file *m,void *v) {struct vga_switcheroo_client *client;
@@ -353,9 +376,15 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)console_unlock (); } +if (vgasr_priv.handler->switch_ddc) { + ret = vgasr_priv.handler->switch_ddc (new_client->id); +if (ret) +return ret; + } + ret = vgasr_priv.handler->switchto (new_client->id);if (ret) -return ret; + goto restore_ddc;if (new_client->ops->reprobe) new_client->ops->reprobe (new_client->pdev);
@@ -367,6 +396,14 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) new_client->active = true;return 0 ; + +restore_ddc: +if (vgasr_priv.handler->switch_ddc) { +int id = (new_client->id ==VGA_SWITCHEROO_IGD ) ? +VGA_SWITCHEROO_DIS :VGA_SWITCHEROO_IGD ; + ret = vgasr_priv.handler->switch_ddc (id); + } +return ret; }static bool check_can_switch (void )
include/linux/vga_switcheroo.h +4 -0
@@ -29,6 +29,7 @@ enum vga_switcheroo_client_id { };struct vga_switcheroo_handler { +int (*switch_ddc)(enum vga_switcheroo_client_id id);int (*switchto)(enum vga_switcheroo_client_id id);int (*power_state)(enum vga_switcheroo_client_id id,enum vga_switcheroo_state state);
@@ -54,6 +55,8 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info); +int vga_switcheroo_switch_ddc (struct pci_dev *pdev); +int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler);void vga_switcheroo_unregister_handler (void );
@@ -72,6 +75,7 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}static inline int vga_switcheroo_register_client (struct pci_dev *dev,const struct vga_switcheroo_client_ops *ops,bool driver_power_control) {return 0 ; }static inline void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info) {} +static inline void vga_switcheroo_switch_ddc (struct pci_dev *pdev) {return NULL ; }static inline int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler) {return 0 ; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops,
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 2/22 vga_switcheroo: Add helper function to get the active client
From: Seth Forshee <[email protected]>
Add vga_switcheroo_get_active_client() to allow drivers to get the active video client. This will be used by drivers wishing to temporarily mux only the DDC to the inactive client.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/vga/vga_switcheroo.c | 14 ++++++++++++++ include/linux/vga_switcheroo.h | 2 ++ 2 files changed, 16 insertions(+)
drivers/gpu/vga/vga_switcheroo.c +14 -0
@@ -209,6 +209,20 @@ find_active_client(struct list_head *head)return NULL ; } +struct pci_dev *vga_switcheroo_get_active_client (void ) +{ +struct vga_switcheroo_client *client; +struct pci_dev *pdev =NULL ; + +mutex_lock (&vgasr_mutex); + client =find_active_client (&vgasr_priv.clients); +if (client) + pdev = client->pdev; +mutex_unlock (&vgasr_mutex); +return pdev; +} +EXPORT_SYMBOL (vga_switcheroo_get_active_client); +int vga_switcheroo_get_client_state (struct pci_dev *pdev) {struct vga_switcheroo_client *client;
include/linux/vga_switcheroo.h +2 -0
@@ -62,6 +62,7 @@ void vga_switcheroo_unregister_handler(void);int vga_switcheroo_process_delayed_switch (void ); +struct pci_dev *vga_switcheroo_get_active_client (void );int vga_switcheroo_get_client_state (struct pci_dev *dev);void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic);
@@ -82,6 +83,7 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, int id, bool active) { return 0; }static inline void vga_switcheroo_unregister_handler (void ) {}static inline int vga_switcheroo_process_delayed_switch (void ) {return 0 ; } +static inline struct pci_dev *vga_switcheroo_get_active_client (void ) {return NULL ; }static inline int vga_switcheroo_get_client_state (struct pci_dev *dev) {return VGA_SWITCHEROO_ON ; }static inline void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic) {}
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 3/22 apple-gmux: Add switch_ddc support
From: Seth Forshee <[email protected]>
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/platform/x86/apple-gmux.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/platform/x86/apple-gmux.c +10 -2
@@ -273,14 +273,21 @@ static const struct backlight_ops gmux_bl_ops = { .update_status = gmux_update_status, }; +static int gmux_switch_ddc (enum vga_switcheroo_client_id id) +{ +if (id ==VGA_SWITCHEROO_IGD ) +gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,1 ); +else +gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,2 ); +return 0 ; +} +static int gmux_switchto (enum vga_switcheroo_client_id id) {if (id ==VGA_SWITCHEROO_IGD ) { -gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,1 );gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DISPLAY ,2 );gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_EXTERNAL ,2 ); }else { -gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,2 );gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DISPLAY ,3 );gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_EXTERNAL ,3 ); }
@@ -347,6 +354,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data) }static struct vga_switcheroo_handler gmux_handler = { + .switch_ddc = gmux_switch_ddc, .switchto = gmux_switchto, .power_state = gmux_set_power_state, .get_client_id = gmux_get_client_id,
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 4/22 drm/edid: Switch DDC when reading the EDID
From: Seth Forshee <[email protected]>
Some dual graphics machines support muxing the DDC separately from the display, so make use of this functionality when reading the EDID on the inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races on the DDC mux state.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/drm_edid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/gpu/drm/drm_edid.c +18 -0
@@ -32,6 +32,7 @@#include <linux/hdmi.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/vga_switcheroo.h> #include <drm/drmP.h> #include <drm/drm_edid.h> #include <drm/drm_displayid.h>
@@ -87,6 +88,8 @@ struct detailed_mode_closure {#define LEVEL_GTF2 2#define LEVEL_CVT 3 +static DEFINE_MUTEX (drm_edid_mutex); + static struct edid_quirk { char vendor[4]; int product_id;
@@ -1377,6 +1380,16 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) {struct edid *edid; +struct pci_dev *pdev = connector->dev->pdev; +struct pci_dev *active_pdev =NULL ; + +mutex_lock (&drm_edid_mutex); + +if (pdev) { + active_pdev =vga_switcheroo_get_active_client (); +if (active_pdev != pdev) +vga_switcheroo_switch_ddc (pdev); + }if (!drm_probe_ddc (adapter))return NULL ;
@@ -1384,6 +1397,11 @@ struct edid *drm_get_edid(struct drm_connector *connector, edid =drm_do_get_edid (connector, drm_do_probe_ddc_edid, adapter);if (edid)drm_get_displayid (connector, edid); + +if (active_pdev && active_pdev != pdev) +vga_switcheroo_switch_ddc (active_pdev); + +mutex_unlock (&drm_edid_mutex);return edid; }EXPORT_SYMBOL (drm_get_edid);
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 5/22 vga_switcheroo: Lock/unlock DDC lines
From: Dave Airlie <[email protected]>
Replace vga_switcheroo_switch_ddc() with vga_switcheroo_lock_ddc() and vga_switcheroo_unlock_ddc(), move mutex from drm_get_edid() to vga_switcheroo.c
Motivation for these changes according to Dave Airlie:
"I can't figure out why I didn't like this, but I rewrote this way back to lock/unlock the ddc lines, bits are contained in this WIP mess. I think I'd prefer something like that otherwise the interface got really ugly."
http://lists.freedesktop.org/archives/dri-devel/2014-June/061629.html
Original commit by Dave Airlie contained additional experimental and unused code. Reduced to the bare minimum and amended with this commit message by Lukas Wunner <[email protected]>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/drm_edid.c | 16 ++------------ drivers/gpu/vga/vga_switcheroo.c | 46 ++++++++++++++++++++++++++++++++++++++-- include/linux/vga_switcheroo.h | 3 ++- 3 files changed, 48 insertions(+), 17 deletions(-)
drivers/gpu/drm/drm_edid.c +2 -14
@@ -88,8 +88,6 @@ struct detailed_mode_closure {#define LEVEL_GTF2 2#define LEVEL_CVT 3 -static DEFINE_MUTEX (drm_edid_mutex); - static struct edid_quirk { char vendor[4]; int product_id;
@@ -1380,16 +1378,8 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) {struct edid *edid; -struct pci_dev *pdev = connector->dev->pdev; -struct pci_dev *active_pdev =NULL ; - -mutex_lock (&drm_edid_mutex); -if (pdev) { - active_pdev =vga_switcheroo_get_active_client (); -if (active_pdev != pdev) -vga_switcheroo_switch_ddc (pdev); - } +vga_switcheroo_lock_ddc (connector->dev->pdev);if (!drm_probe_ddc (adapter))return NULL ;
@@ -1398,10 +1388,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,if (edid)drm_get_displayid (connector, edid); -if (active_pdev && active_pdev != pdev) -vga_switcheroo_switch_ddc (active_pdev); +vga_switcheroo_unlock_ddc (connector->dev->pdev); -mutex_unlock (&drm_edid_mutex);return edid; }EXPORT_SYMBOL (drm_get_edid);
drivers/gpu/vga/vga_switcheroo.c +44 -2
77 lines not on this page · raw message
include/linux/vga_switcheroo.h +2 -1
@@ -55,7 +55,8 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info); -int vga_switcheroo_switch_ddc (struct pci_dev *pdev); +int vga_switcheroo_lock_ddc (struct pci_dev *pdev); +int vga_switcheroo_unlock_ddc (struct pci_dev *pdev);int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler);void vga_switcheroo_unregister_handler (void );
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 6/22 vga_switcheroo: Lock/unlock DDC lines harder
Unlock DDC lines if drm_probe_ddc() fails.
If the inactive client registers before the active client then old_ddc_owner cannot be determined with find_active_client() (null pointer dereference). Therefore change semantics of the ->switch_ddc handler callback to return old_ddc_owner on success or a negative int on failure.
Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to avoid locking inversion when one client locks vgasr_mutex on entering vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another client is holding ddc_lock and tries to acquire vgasr_mutex on entering vga_switcheroo_unlock_ddc().
Lock ddc_lock in stage2 to avoid race condition where reading the EDID and switching happens simultaneously.
Switch DDC lines on MIGD / MDIS commands and on runtime suspend.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/drm_edid.c | 9 ++-- drivers/gpu/vga/vga_switcheroo.c | 110 ++++++++++++++++++++++---------------- drivers/platform/x86/apple-gmux.c | 15 +++++- include/linux/vga_switcheroo.h | 3 +- 4 files changed, 87 insertions(+), 50 deletions(-)
drivers/gpu/drm/drm_edid.c +6 -3
@@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) {struct edid *edid; +struct pci_dev *pdev = connector->dev->pdev; -vga_switcheroo_lock_ddc (connector->dev->pdev); +vga_switcheroo_lock_ddc (pdev); -if (!drm_probe_ddc (adapter)) +if (!drm_probe_ddc (adapter)) { +vga_switcheroo_unlock_ddc (pdev);return NULL ; + } edid =drm_do_get_edid (connector, drm_do_probe_ddc_edid, adapter);if (edid)drm_get_displayid (connector, edid); -vga_switcheroo_unlock_ddc (connector->dev->pdev); +vga_switcheroo_unlock_ddc (pdev);return edid; }
drivers/gpu/vga/vga_switcheroo.c +65 -45
182 lines not on this page · raw message
drivers/platform/x86/apple-gmux.c +14 -1
@@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = {static int gmux_switch_ddc (enum vga_switcheroo_client_id id) { +enum vga_switcheroo_client_id old_ddc_owner; + +if (gmux_read8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ) ==1 ) + old_ddc_owner =VGA_SWITCHEROO_IGD ; +else + old_ddc_owner =VGA_SWITCHEROO_DIS ; + +pr_debug ("Switching gmux DDC from %d to %d\n" , old_ddc_owner, id); + +if (id == old_ddc_owner) +return old_ddc_owner; +if (id ==VGA_SWITCHEROO_IGD )gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,1 );else gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,2 ); -return 0 ; + +return old_ddc_owner; }static int gmux_switchto (enum vga_switcheroo_client_id id)
include/linux/vga_switcheroo.h +2 -1
@@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}static inline int vga_switcheroo_register_client (struct pci_dev *dev,const struct vga_switcheroo_client_ops *ops,bool driver_power_control) {return 0 ; }static inline void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info) {} -static inline void vga_switcheroo_switch_ddc (struct pci_dev *pdev) {return NULL ; } +static inline int vga_switcheroo_lock_ddc (struct pci_dev *pdev) {return -ENODEV ; } +static inline int vga_switcheroo_unlock_ddc (struct pci_dev *pdev) {return -ENODEV ; }static inline int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler) {return 0 ; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops,
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 7/22 Revert "vga_switcheroo: Add helper function to get the active client"
This reverts commit 26814ce68904c9faf977c90edac798156311981f.
The helper function is no longer needed after Dave Airlie's rewrite of vga_switcheroo_switch_ddc(), the commit introducing it was only included because 31f23c3d488e ("drm/edid: Switch DDC when reading the EDID") does not compile without it.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/vga/vga_switcheroo.c | 14 -------------- include/linux/vga_switcheroo.h | 2 -- 2 files changed, 16 deletions(-)
drivers/gpu/vga/vga_switcheroo.c +0 -14
@@ -214,20 +214,6 @@ find_active_client(struct list_head *head)return NULL ; } -struct pci_dev *vga_switcheroo_get_active_client (void ) -{ -struct vga_switcheroo_client *client; -struct pci_dev *pdev =NULL ; - -mutex_lock (&vgasr_mutex); - client =find_active_client (&vgasr_priv.clients); -if (client) - pdev = client->pdev; -mutex_unlock (&vgasr_mutex); -return pdev; -} -EXPORT_SYMBOL (vga_switcheroo_get_active_client); -int vga_switcheroo_get_client_state (struct pci_dev *pdev) {struct vga_switcheroo_client *client;
include/linux/vga_switcheroo.h +0 -2
@@ -63,7 +63,6 @@ void vga_switcheroo_unregister_handler(void);int vga_switcheroo_process_delayed_switch (void ); -struct pci_dev *vga_switcheroo_get_active_client (void );int vga_switcheroo_get_client_state (struct pci_dev *dev);void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic);
@@ -85,7 +84,6 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, int id, bool active) { return 0; }static inline void vga_switcheroo_unregister_handler (void ) {}static inline int vga_switcheroo_process_delayed_switch (void ) {return 0 ; } -static inline struct pci_dev *vga_switcheroo_get_active_client (void ) {return NULL ; }static inline int vga_switcheroo_get_client_state (struct pci_dev *dev) {return VGA_SWITCHEROO_ON ; }static inline void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic) {}
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 8/22 Revert "vga_switcheroo: add reprobe hook for fbcon to recheck connected outputs."
This reverts commit 8d608aa6295242fe4c4b6105b8c59c6a5b232d89.
Reprobing must happen asynchronously as the driver may grab vgasr_mutex when invoking vga_switcheroo_client_fb_set(), when locking the DDC lines or when using the upcoming DDC/AUX proxy functionality. And that lock is already held in stage2 by vga_switcheroo_debugfs_write().
Infrastructure for asynchronous reprobing is added to vga_switcheroo with a subsequent commit and could easily be used in stage2, but:
- The ->reprobe callback was only ever used by a single driver, nouveau.
- Drivers need to reprobe their outputs anyway after waking up, this is nothing specific to dual GPU laptops and should be added to the resume code of the driver.
- It would seem there's no need anymore to delay reprobing until after switching the mux: We have two methods now to make reprobing work with the display not switched (switching only the DDC lines and proxying), at least one of them should always work.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 - drivers/gpu/drm/i915/i915_dma.c | 1 - drivers/gpu/drm/nouveau/nouveau_vga.c | 8 -------- drivers/gpu/drm/radeon/radeon_device.c | 1 - drivers/gpu/vga/vga_switcheroo.c | 3 --- include/linux/vga_switcheroo.h | 1 - 6 files changed, 15 deletions(-)
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -1
@@ -1076,7 +1076,6 @@ static bool amdgpu_switcheroo_can_switch(struct pci_dev *pdev)static const struct vga_switcheroo_client_ops amdgpu_switcheroo_ops = { .set_gpu_state = amdgpu_switcheroo_set_state, - .reprobe =NULL , .can_switch = amdgpu_switcheroo_can_switch, };
drivers/gpu/drm/i915/i915_dma.c +0 -1
@@ -388,7 +388,6 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { .set_gpu_state = i915_switcheroo_set_state, - .reprobe =NULL , .can_switch = i915_switcheroo_can_switch, };
drivers/gpu/drm/nouveau/nouveau_vga.c +0 -8
@@ -55,13 +55,6 @@ nouveau_switcheroo_set_state(struct pci_dev *pdev, } } -static void -nouveau_switcheroo_reprobe (struct pci_dev *pdev) -{ -struct drm_device *dev =pci_get_drvdata (pdev); -nouveau_fbcon_output_poll_changed (dev); -} -static bool nouveau_switcheroo_can_switch (struct pci_dev *pdev) {
@@ -78,7 +71,6 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)static const struct vga_switcheroo_client_ops nouveau_switcheroo_ops = { .set_gpu_state = nouveau_switcheroo_set_state, - .reprobe = nouveau_switcheroo_reprobe, .can_switch = nouveau_switcheroo_can_switch, };
drivers/gpu/drm/radeon/radeon_device.c +0 -1
@@ -1258,7 +1258,6 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = { .set_gpu_state = radeon_switcheroo_set_state, - .reprobe =NULL , .can_switch = radeon_switcheroo_can_switch, };
drivers/gpu/vga/vga_switcheroo.c +0 -3
@@ -440,9 +440,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)return ret; } -if (new_client->ops->reprobe) - new_client->ops->reprobe (new_client->pdev); -if (active->pwr_state ==VGA_SWITCHEROO_ON )vga_switchoff (active);
include/linux/vga_switcheroo.h +0 -1
@@ -39,7 +39,6 @@ struct vga_switcheroo_handler {struct vga_switcheroo_client_ops {void (*set_gpu_state)(struct pci_dev *dev,enum vga_switcheroo_state ); -void (*reprobe)(struct pci_dev *dev);bool (*can_switch)(struct pci_dev *dev); };
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 9/22 drm/nouveau: Lock/unlock DDC lines on probe
On dual gpu laptops such as the MacBook Pro, ask vga_switcheroo to switch the DDC lines to the Nvidia gpu before probing them.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/nouveau/nouveau_connector.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/nouveau/nouveau_connector.c +6 -1
@@ -27,6 +27,7 @@#include <acpi/button.h> #include <linux/pm_runtime.h> +#include <linux/vga_switcheroo.h> #include <drm/drmP.h> #include <drm/drm_edid.h>
@@ -148,7 +149,11 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)break ; }else if (nv_encoder->i2c) { -if (nv_probe_i2c (nv_encoder->i2c,0x50 )) +int ret; +vga_switcheroo_lock_ddc (dev->pdev); + ret =nv_probe_i2c (nv_encoder->i2c,0x50 ); +vga_switcheroo_unlock_ddc (dev->pdev); +if (ret)break ; } }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 10/22 apple-gmux: Assign apple_gmux_data before registering
From: Matthew Garrett <[email protected]>
Registering the handler after both GPUs will trigger a DDC switch for connector reprobing. This will oops if apple_gmux_data hasn't already been assigned. Reorder the code to do that.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/platform/x86/apple-gmux.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/platform/x86/apple-gmux.c +6 -4
@@ -609,18 +609,20 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) gmux_data->gpe =-1 ; } + apple_gmux_data = gmux_data; +init_completion (&gmux_data->powerchange_done); +gmux_enable_interrupts (gmux_data); +if (vga_switcheroo_register_handler (&gmux_handler)) { ret = -ENODEV ; goto err_register_handler; } -init_completion (&gmux_data->powerchange_done); - apple_gmux_data = gmux_data; -gmux_enable_interrupts (gmux_data); -return 0 ; err_register_handler: +gmux_disable_interrupts (gmux_data); + apple_gmux_data =NULL ;if (gmux_data->gpe >=0 )acpi_disable_gpe (NULL , gmux_data->gpe); err_enable_gpe:
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 11/22 vga_switcheroo: Generate hotplug event on handler and proxy registration
On laptops which require the handler to switch DDC lines, already registered clients must reprobe their connectors if the handler registers late. This is the case on pre-retina MacBook Pros, which use a gmux controller as handler.
Based (loosely) on a patch by Matthew Garrett <[email protected]> who used an additional driver callback rather than generating a hotplug event:
http://lists.freedesktop.org/archives/dri-devel/2014-June/060941.html
Matthew Garrett invoked the driver callback in vga_switcheroo_enable(). On pre-retina MacBook Pros, this delayed reprobing until a second gpu registers, which may never happen if its driver is not installed or blacklisted. The MacBook Pro 2011 in particular suffers from widespread gpu soldering issues due to overheating, eventually rendering it unusable with OS X. Folks are solving this by moving to Linux and disabling the discrete gpu entirely. In those cases we can't wait for a second gpu to register, we do need to reprobe as soon as the handler registers:
https://ifixit.org/blog/6882/why-i-drilled-holes-in-my-macbook-pro-and-put-it-in-the-oven/#comment-26745
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/vga/vga_switcheroo.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)
drivers/gpu/vga/vga_switcheroo.c +34 -0
@@ -18,6 +18,8 @@- can_switch - check if the device is in a position to switch now */ +#include <drm/drm_crtc_helper.h> +#include <linux/module.h> #include <linux/seq_file.h> #include <linux/uaccess.h>
@@ -35,6 +37,7 @@struct vga_switcheroo_client {struct pci_dev *pdev;struct fb_info *fb_info; +struct work_struct reprobe_work;int pwr_state;const struct vga_switcheroo_client_ops *ops;int id;
@@ -106,6 +109,30 @@ static void vga_switcheroo_enable(void) vgasr_priv.active = true; } +static void vga_switcheroo_reprobe_client (struct work_struct *work) +{ +struct vga_switcheroo_client *client = + container_of(work, struct vga_switcheroo_client, reprobe_work); +void (*generate_hotplug_event)(struct drm_device *); + + generate_hotplug_event =symbol_get (drm_kms_helper_hotplug_event); +if (!generate_hotplug_event) +return ; + +generate_hotplug_event (pci_get_drvdata (client->pdev)); +} + +static void vga_switcheroo_reprobe_inactive_clients (void ) +{ +struct vga_switcheroo_client *client; + +list_for_each_entry (client, &vgasr_priv.clients, list) +if (!client->active &&client_is_vga (client)) { +cancel_work_sync (&client->reprobe_work); +schedule_work (&client->reprobe_work); + } +} +int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler) {mutex_lock (&vgasr_mutex);
@@ -115,11 +142,17 @@ int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) } vgasr_priv.handler = handler; +if (vga_switcheroo_ready ()) {printk (KERN_INFO "vga_switcheroo: enabled\n" );vga_switcheroo_enable (); }mutex_unlock (&vgasr_mutex); + +/* clients which registered before the handler must reprobe */ +if (vgasr_priv.handler->switch_ddc) +vga_switcheroo_reprobe_inactive_clients (); +return 0 ; }EXPORT_SYMBOL (vga_switcheroo_register_handler);
@@ -153,6 +186,7 @@ static int register_client(struct pci_dev *pdev, client->id = id; client->active = active; client->driver_power_control = driver_power_control; +INIT_WORK (&client->reprobe_work, vga_switcheroo_reprobe_client);mutex_lock (&vgasr_mutex);list_add_tail (&client->list, &vgasr_priv.clients);
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 12/22 drm/i915: Preserve SSC earlier
Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3") added code to intel_modeset_gem_init to override the SSC status read from VBT with the SSC status set by BIOS.
However, intel_modeset_gem_init is invoked *after* intel_modeset_init, which calls intel_setup_outputs, which *modifies* SSC status by way of intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init doesn't preserve the SSC status set by BIOS but whatever intel_init_pch_refclk decided on.
This is a problem on dual gpu laptops such as the MacBook Pro which require either a handler to switch DDC lines, or the discrete gpu to proxy DDC/AUX communication: Both the handler and the discrete gpu may initialize after the i915 driver, and consequently, an LVDS connector may initially seem disconnected and the SSC therefore is disabled by intel_init_pch_refclk, but on reprobe the connector may turn out to be connected and the SSC must then be enabled.
Due to 92122789b2d6 however, the SSC is not enabled on reprobe since it is assumed BIOS disabled it while in fact it was disabled by intel_init_pch_refclk.
Also, because the SSC status is preserved so late, the preserved value only ever gets used on resume but not on panel initialization:intel_modeset_init calls intel_init_display which indirectly calls intel_panel_use_ssc via multiple subroutines, *before* the BIOS value overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc is the sole user of dev_priv->vbt.lvds_use_ssc).
Fix this by moving the code introduced by 92122789b2d6 from intel_modeset_gem_init to intel_modeset_init before the invocation of intel_setup_outputs and intel_init_display.
Add a DRM_DEBUG_KMS as suggested way back by Jani:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)
drivers/gpu/drm/i915/intel_display.c +18 -11
@@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)if (INTEL_INFO (dev)->num_pipes ==0 )return ; +/* +* There may be no VBT; and if the BIOS enabled SSC we can +* just keep using it to avoid unnecessary flicker. Whereas if the +* BIOS isn't using it, don't assume it will work even if the VBT +* indicates as much. +*/ +if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) { +bool bios_lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & +DREF_SSC1_ENABLE ); + +if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) { +DRM_DEBUG_KMS ("SSC %sabled by BIOS, overriding VBT which says %sabled\n" , + bios_lvds_use_ssc ?"en" :"dis" , + dev_priv->vbt.lvds_use_ssc ?"en" :"dis" ); + dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc; + } + } +intel_init_display (dev);intel_init_audio (dev);
@@ -15446,7 +15464,6 @@ err:void intel_modeset_gem_init (struct drm_device *dev) { -struct drm_i915_private *dev_priv = dev->dev_private;struct drm_crtc *c;struct drm_i915_gem_object *obj;int ret;
@@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)intel_init_gt_powersave (dev);mutex_unlock (&dev->struct_mutex); -/* -* There may be no VBT; and if the BIOS enabled SSC we can -* just keep using it to avoid unnecessary flicker. Whereas if the -* BIOS isn't using it, don't assume it will work even if the VBT -* indicates as much. -*/ -if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) - dev_priv->vbt.lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & -DREF_SSC1_ENABLE ); -intel_modeset_init_hw (dev);intel_setup_overlay (dev);
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 13/22 drm/i915: Reprobe eDP and LVDS connectors on hotplug event
The i915 driver probes eDP and LVDS connectors once on startup by invoking intel_setup_outputs(). If no DPCD or EDID can be obtained, it will remove the connectors from the device's mode configuration, presuming they're ghost connectors. As a result, subsequent calls to drm_fb_helper_hotplug_event() won't be able to pick up changes on these connectors.
This is a problem on dual gpu laptops such as the MacBook Pro which require either a handler to switch DDC lines, or the discrete gpu to proxy the DDC/AUX communication: Both the handler and the discrete gpu may initialize after the i915 driver, and consequently, eDP and LVDS connectors which may seem disconnected at startup may later turn out to be connected.
By contrast, nouveau will keep eDP and LVDS connectors for which no modes were found in the device's mode configuration and thus is able to reprobe these connectors in drm_fb_helper_hotplug_event().
Assimilate to nouveau's behaviour: Keep modeless eDP and LVDS connectors in the mode configuration and change the ->output_poll_changed callback to reprobe them on hotplug events.
In the case of LVDS, split intel_lvds_init() in half: The first portion is executed once on startup. This consists of detecting, setting up and registering the connector. The second portion is executed both on startup and on every reprobe. This consists of reading the panel's EDID, determining if dual channel LVDS is used, and initializing the reference clock.
In the case of eDP, reprobe involves calling intel_edp_init_connector() and initializing the reference clock.
Based (loosely) on a patch by Matthew Garrett <[email protected]> who duplicated intel_setup_outputs() and reduced it to just the eDP probing portion (which is not sufficient since pre-retina MBPs used LVDS):
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0024-i915-Add-support-for-reprobing-for-a-panel.patch
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/i915/intel_display.c | 41 ++++++++++++++++++---- drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++-------- drivers/gpu/drm/i915/intel_drv.h | 6 ++++ drivers/gpu/drm/i915/intel_lvds.c | 67 +++++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_panel.c | 4 +-- 5 files changed, 112 insertions(+), 44 deletions(-)
drivers/gpu/drm/i915/intel_display.c +35 -6
@@ -8223,11 +8223,11 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)for_each_intel_encoder (dev, encoder) {switch (encoder->type) {case INTEL_OUTPUT_LVDS : - has_panel = true; + has_panel =intel_lvds_has_panel (encoder); has_lvds = true;break ;case INTEL_OUTPUT_EDP : - has_panel = true; + has_panel =intel_edp_has_panel (encoder);if (enc_to_dig_port (&encoder->base)->port ==PORT_A ) has_cpu_edp = true;break ;
@@ -14478,15 +14478,44 @@ intel_user_framebuffer_create(struct drm_device *dev,return intel_framebuffer_create (dev, mode_cmd, obj); } -#ifndef CONFIG_DRM_I915_FBDEV -static inline void intel_fbdev_output_poll_changed (struct drm_device *dev) +static void intel_output_poll_changed (struct drm_device *dev) { -} +struct drm_i915_private *dev_priv = dev->dev_private; +struct intel_connector *intel_connector; + +/* Reprobe LVDS and eDP as long as no EDID was retrieved from panel */ +for_each_intel_connector (dev, intel_connector) { +struct drm_connector *connector = &intel_connector->base; + +if ((connector->connector_type !=DRM_MODE_CONNECTOR_LVDS && + connector->connector_type != DRM_MODE_CONNECTOR_eDP) || + !IS_ERR_OR_NULL (intel_connector->edid)) +continue ; + +if ((connector->connector_type ==DRM_MODE_CONNECTOR_LVDS && + !intel_lvds_probe_modes (connector)) || + (connector->connector_type == DRM_MODE_CONNECTOR_eDP && + !intel_edp_init_connector ( +enc_to_intel_dp (&intel_connector->encoder->base), + intel_connector))) +continue ; + +intel_init_pch_refclk (dev); +drm_helper_move_panel_connectors_to_head (dev); +mutex_lock (&dev_priv->backlight_lock); +if (intel_connector->panel.backlight.device ==NULL ) +intel_backlight_device_register (intel_connector); +mutex_unlock (&dev_priv->backlight_lock); + } + +#ifdef CONFIG_DRM_I915_FBDEV +intel_fbdev_output_poll_changed (dev);#endif +}static const struct drm_mode_config_funcs intel_mode_funcs = { .fb_create = intel_user_framebuffer_create, - .output_poll_changed = intel_fbdev_output_poll_changed, + .output_poll_changed = intel_output_poll_changed, .atomic_check = intel_atomic_check, .atomic_commit = intel_atomic_commit, .atomic_state_alloc = intel_atomic_state_alloc,
drivers/gpu/drm/i915/intel_dp.c +23 -15
@@ -1998,6 +1998,15 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)pps_unlock (intel_dp); } +bool intel_edp_has_panel (struct intel_encoder *intel_encoder) +{ +struct intel_dp *intel_dp =enc_to_intel_dp (&intel_encoder->base); +struct intel_connector *intel_connector = intel_dp->attached_connector; + +return intel_dp->dpcd[DP_DPCD_REV ] !=0 && + intel_connector->panel.fixed_mode !=NULL ; +} +/* Enable backlight in the panel power control. */ static void _intel_edp_backlight_on (struct intel_dp *intel_dp) {
@@ -4338,6 +4347,9 @@ edp_detect(struct intel_dp *intel_dp)struct drm_device *dev =intel_dp_to_dev (intel_dp);enum drm_connector_status status; +if (!intel_edp_has_panel (intel_dp->attached_connector->encoder)) +return connector_status_disconnected; + status =intel_panel_detect (dev);if (status == connector_status_unknown) status = connector_status_connected;
@@ -5599,8 +5611,8 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,return downclock_mode; } -static bool intel_edp_init_connector (struct intel_dp *intel_dp, -struct intel_connector *intel_connector) +bool intel_edp_init_connector (struct intel_dp *intel_dp, +struct intel_connector *intel_connector) {struct drm_connector *connector = &intel_connector->base;struct intel_digital_port *intel_dig_port =dp_to_dig_port (intel_dp);
@@ -5614,9 +5626,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,struct edid *edid;enum pipe pipe =INVALID_PIPE ; -if (!is_edp (intel_dp)) -return true; -pps_lock (intel_dp);intel_edp_panel_vdd_sanitize (intel_dp);pps_unlock (intel_dp);
@@ -5630,8 +5639,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, intel_dp->dpcd[DP_MAX_DOWNSPREAD ] &DP_NO_AUX_HANDSHAKE_LINK_TRAINING ; }else { -/* if this fails, presume the device is a ghost */ -DRM_INFO ("failed to retrieve link info, disabling eDP\n" ); +DRM_INFO ("failed to retrieve eDP link info\n" );return false; }
@@ -5676,8 +5684,12 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,mutex_unlock (&dev->mode_config.mutex);if (IS_VALLEYVIEW (dev)) { - intel_dp->edp_notifier.notifier_call = edp_notify_handler; -register_reboot_notifier (&intel_dp->edp_notifier); +if (intel_dp->edp_notifier.notifier_call ==NULL ) { + intel_dp->edp_notifier.notifier_call = + edp_notify_handler; +if (register_reboot_notifier (&intel_dp->edp_notifier)) + intel_dp->edp_notifier.notifier_call =NULL ; + }/* * Figure out the current pipe for the initial backlight setup.
@@ -5817,9 +5829,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,intel_dp_mst_encoder_init (intel_dig_port, intel_connector->base.base.id); -if (!intel_edp_init_connector (intel_dp, intel_connector)) { -drm_dp_aux_unregister (&intel_dp->aux); -if (is_edp (intel_dp)) { +if (is_edp (intel_dp)) { +if (!intel_edp_init_connector (intel_dp, intel_connector)) {cancel_delayed_work_sync (&intel_dp->panel_vdd_work);/* * vdd might still be enabled do to the delayed vdd off.
@@ -5829,9 +5840,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,edp_panel_vdd_off_sync (intel_dp);pps_unlock (intel_dp); } -drm_connector_unregister (connector); -drm_connector_cleanup (connector); -return false; }intel_dp_add_properties (intel_dp, connector);
drivers/gpu/drm/i915/intel_drv.h +6 -0
@@ -1171,11 +1171,14 @@ bool intel_dp_compute_config(struct intel_encoder *encoder,bool intel_dp_is_edp (struct drm_device *dev,enum port port);enum irqreturn intel_dp_hpd_pulse (struct intel_digital_port *intel_dig_port,bool long_hpd); +bool intel_edp_init_connector (struct intel_dp *intel_dp, +struct intel_connector *intel_connector);void intel_edp_backlight_on (struct intel_dp *intel_dp);void intel_edp_backlight_off (struct intel_dp *intel_dp);void intel_edp_panel_vdd_on (struct intel_dp *intel_dp);void intel_edp_panel_on (struct intel_dp *intel_dp);void intel_edp_panel_off (struct intel_dp *intel_dp); +bool intel_edp_has_panel (struct intel_encoder *intel_encoder);void intel_dp_add_properties (struct intel_dp *intel_dp,struct drm_connector *connector);void intel_dp_mst_suspend (struct drm_device *dev);void intel_dp_mst_resume (struct drm_device *dev);
@@ -1258,6 +1261,8 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,/* intel_lvds.c */ void intel_lvds_init (struct drm_device *dev); +bool intel_lvds_probe_modes (struct drm_connector *connector); +bool intel_lvds_has_panel (struct intel_encoder *intel_encoder);bool intel_is_dual_link_lvds (struct drm_device *dev);
@@ -1307,6 +1312,7 @@ extern struct drm_display_mode *intel_find_panel_downclock( struct drm_connector *connector);void intel_backlight_register (struct drm_device *dev);void intel_backlight_unregister (struct drm_device *dev); +int intel_backlight_device_register (struct intel_connector *connector);/* intel_psr.c */
drivers/gpu/drm/i915/intel_lvds.c +46 -21
109 lines not on this page · raw message
drivers/gpu/drm/i915/intel_panel.c +2 -2
@@ -1139,7 +1139,7 @@ static const struct backlight_ops intel_backlight_device_ops = { .get_brightness = intel_backlight_device_get_brightness, }; -static int intel_backlight_device_register (struct intel_connector *connector) +int intel_backlight_device_register (struct intel_connector *connector) {struct intel_panel *panel = &connector->panel;struct backlight_properties props;
@@ -1202,7 +1202,7 @@ static void intel_backlight_device_unregister(struct intel_connector *connector) } }#else /* CONFIG_BACKLIGHT_CLASS_DEVICE */ -static int intel_backlight_device_register (struct intel_connector *connector) +int intel_backlight_device_register (struct intel_connector *connector) {return 0 ; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lukas Wunner
reviewerPATCH RESEND v2 14/22 drm/i915: Fix failure paths around initial fbdev allocation
From: Tvrtko Ursulin <[email protected]>
We had two failure modes here:
1.
Deadlock in intelfb_alloc failure path where it calls drm_framebuffer_remove, which grabs the struct mutex and intelfb_create (caller of intelfb_alloc) was already holding it.
2.
Deadlock in intelfb_create failure path where it calls drm_framebuffer_unreference, which grabs the struct mutex and intelfb_create was already holding it.
v2:
* Reformat commit msg to 72 chars. (Lukas Wunner)
* Add third failure mode. (Lukas Wunner)
v3:
* On fb alloc failure, unref gem object where it gets refed, fix double unref in separate commit. (Ville Syrjälä)
v4:
* Lock struct mutex on unref. (Chris Wilson)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
framebuffer_references--")
--- drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/gpu/drm/i915/intel_fbdev.c +12 -8
@@ -140,7 +140,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, {struct intel_fbdev *ifbdev = container_of(helper, struct intel_fbdev, helper); -struct drm_framebuffer *fb; +struct drm_framebuffer *fb =NULL ;struct drm_device *dev = helper->dev;struct drm_mode_fb_cmd2 mode_cmd = {};struct drm_i915_gem_object *obj;
@@ -158,6 +158,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper, mode_cmd.pixel_format =drm_mode_legacy_fb_format (sizes->surface_bpp, sizes->surface_depth); +mutex_lock (&dev->struct_mutex); + size = mode_cmd.pitches[0 ] * mode_cmd.height; size =PAGE_ALIGN (size); obj =i915_gem_object_create_stolen (dev, size);
@@ -179,18 +181,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper, ret =intel_pin_and_fence_fb_obj (NULL , fb,NULL ,NULL ,NULL );if (ret) {DRM_ERROR ("failed to pin obj: %d\n" , ret); - goto out_fb; + goto out_unref; } +mutex_unlock (&dev->struct_mutex); + ifbdev->fb =to_intel_framebuffer (fb);return 0 ; -out_fb: -drm_framebuffer_remove (fb); out_unref:drm_gem_object_unreference (&obj->base); out: +mutex_unlock (&dev->struct_mutex); +if (fb) +drm_framebuffer_remove (fb);return ret; }
@@ -208,8 +213,6 @@ static int intelfb_create(struct drm_fb_helper *helper,int size, ret;bool prealloc = false; -mutex_lock (&dev->struct_mutex); -if (intel_fb && (sizes->fb_width > intel_fb->base.width || sizes->fb_height > intel_fb->base.height)) {
@@ -224,7 +227,7 @@ static int intelfb_create(struct drm_fb_helper *helper,DRM_DEBUG_KMS ("no BIOS fb, allocating a new one\n" ); ret =intelfb_alloc (helper, sizes);if (ret) - goto out_unlock; +return ret; intel_fb = ifbdev->fb; }else {DRM_DEBUG_KMS ("re-using BIOS fb\n" );
@@ -236,6 +239,8 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +mutex_lock (&dev->struct_mutex); + info =framebuffer_alloc (0 , &dev->pdev->dev);if (!info) { ret = -ENOMEM ;
@@ -306,7 +311,6 @@ static int intelfb_create(struct drm_fb_helper *helper, out_unpin:i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); -out_unlock:mutex_unlock (&dev->struct_mutex);return ret; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH RESEND v2 15/22 drm/i915: On fb alloc failure, unref gem object where it gets refed
Currently when allocating a framebuffer fails, the gem object gets unrefed at the bottom of the call chain in __intel_framebuffer_create, not where it gets refed, which is in intel_framebuffer_create_for_mode (via i915_gem_alloc_object) and in intel_user_framebuffer_create (via drm_gem_object_lookup).
This invites mistakes: As discovered by Tvrtko Ursulin, a double unref has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
As suggested by Ville Syrjälä, improve code clarity by moving the unref away from __intel_framebuffer_create to where the gem object gets refed.
[Changelog is in preceding patch by Tvrtko Ursulin.]
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
allocation")
Cc: Chris Wilson <[email protected]>
Cc: Tvrtko Ursulin <[email protected]>
Cc: Ville Syrjälä <[email protected]>
--- drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
drivers/gpu/drm/i915/intel_display.c +14 -7
@@ -10145,20 +10145,17 @@ __intel_framebuffer_create(struct drm_device *dev,int ret; intel_fb =kzalloc (sizeof (*intel_fb),GFP_KERNEL ); -if (!intel_fb) { -drm_gem_object_unreference (&obj->base); +if (!intel_fb)return ERR_PTR (-ENOMEM ); - } ret =intel_framebuffer_init (dev, intel_fb, mode_cmd, obj);if (ret) goto err;return &intel_fb->base; + err: -drm_gem_object_unreference (&obj->base);kfree (intel_fb); -return ERR_PTR (ret); }
@@ -10198,6 +10195,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,struct drm_display_mode *mode, int depth, int bpp) { + struct drm_framebuffer *fb;struct drm_i915_gem_object *obj;struct drm_mode_fb_cmd2 mode_cmd = {0 };
@@ -10212,7 +10210,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev, bpp); mode_cmd.pixel_format =drm_mode_legacy_fb_format (bpp, depth); -return intel_framebuffer_create (dev, &mode_cmd, obj); + fb =intel_framebuffer_create (dev, &mode_cmd, obj); +if (IS_ERR (fb)) +drm_gem_object_unreference_unlocked (&obj->base); + +return fb; } static struct drm_framebuffer *
@@ -14468,6 +14470,7 @@ intel_user_framebuffer_create(struct drm_device *dev,struct drm_file *filp, struct drm_mode_fb_cmd2 *mode_cmd) { + struct drm_framebuffer *fb;struct drm_i915_gem_object *obj; obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -14475,7 +14478,11 @@ intel_user_framebuffer_create(struct drm_device *dev,if (&obj->base ==NULL )return ERR_PTR (-ENOENT ); -return intel_framebuffer_create (dev, mode_cmd, obj); + fb =intel_framebuffer_create (dev, mode_cmd, obj); +if (IS_ERR (fb)) +drm_gem_object_unreference_unlocked (&obj->base); + +return fb; }static void intel_output_poll_changed (struct drm_device *dev)
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lukas Wunner
reviewerPATCH v2 16/22 drm: Create new fb and replace default 1024x768 fb on hotplug event
If no connectors with modes are found, drm_fb_helper_single_fb_probe() will allocate a default 1024x768 fb. This becomes a problem if one of the connectors subsequently changes to connected status: We're stuck with the default fb even if the display allows a larger resolution (or doesn't support 1024x768 at all) since modes larger than the existing fb will be deemed ineligible by drm_fb_helper_hotplug_event().
One relevant use case are dual gpu laptops such as the MacBook Pro which require either a handler to switch DDC lines, or the active gpu to proxy the DDC/AUX communication: Both the handler and the active gpu may register late, and consequently, the inactive gpu's eDP and LVDS connectors may seem disconnected at startup but may later on turn out to be connected.
Although primarily aimed at vga_switcheroo setups, the patch may come in handy in other use cases: One example is a desktop machine that is (inadvertantly or otherwise) booted headless, and the user later on plugs in a display. Another example is unplugging of the display by the user and replacement with a larger one.
Solve this by checking if there aren't any modes at all. If so, don't constrain probed modes to the existing fb size but rather allow them to occupy the maximum surface size. After probing, check if we found any modes. If we did, discard the existing fb and recreate it from scratch by invoking the driver's ->fb_probe callback by way of drm_fb_helper_single_fb_probe().
That function is normally called only once on driver initialization. The fb_helper is kept so ensure that it's not added once more to the panic handler list.
Because multiple hotplug events may fire simultaneously, protect connector probing and fb recreation with drm_modeset_lock_all(). In particular, drm_fb_helper_hotplug_event() may itself fire another hotplug event by way of drm_fb_helper_probe_connector_modes(), which invokes the ->fill_modes callback, which for most drivers is drm_helper_probe_single_connector_modes(), which schedules output_poll_execute().
Amend the ->fb_probe callback in the i915, nouveau and radeon drivers to discard an existing fb. Because the _destroy() function is now used further up in intel_fbdev.c, nouveau_fbcon.c and radeon_fb.c, add a prototype for it. This is intended to ease reviewing, the prototype shall be replaced by the body of the function before this gets merged.
Drivers other than these three will just create an additional fb under the limited circumstances described above, which wastes memory but is otherwise "mostly harmless".
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/drm_fb_helper.c | 41 ++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_fbdev.c | 26 ++++++++++----------- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 10 +++++++- drivers/gpu/drm/radeon/radeon_fb.c | 11 ++++++--- 4 files changed, 63 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +33 -8
@@ -1097,7 +1097,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,register_sysrq_key ('v' , &sysrq_drm_fb_helper_restore_op); } -list_add (&fb_helper->kernel_fb_list, &kernel_fb_helper_list); +if (list_empty (&fb_helper->kernel_fb_list)) +list_add (&fb_helper->kernel_fb_list, &kernel_fb_helper_list);return 0 ; }
@@ -1770,23 +1771,47 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) {struct drm_device *dev = fb_helper->dev;u32 max_width, max_height; +bool modes_before_probe = false; +bool modes_after_probe = false; +int i; -mutex_lock (&fb_helper->dev->mode_config.mutex); +drm_modeset_lock_all (dev);if (!fb_helper->fb || !drm_fb_helper_is_bound (fb_helper)) { fb_helper->delayed_hotplug = true; -mutex_unlock (&fb_helper->dev->mode_config.mutex); +drm_modeset_unlock_all (dev);return 0 ; }DRM_DEBUG_KMS ("\n" ); - max_width = fb_helper->fb->width; - max_height = fb_helper->fb->height; +/* If so far we have no modes and thus only the default 1024x768 fb, +* allow probed modes to occupy the maximum surface size */ +for (i =0 ; i < fb_helper->crtc_count; i++) +if (fb_helper->crtc_info[i].desired_mode) { + modes_before_probe = true; +break ; + } +if (!modes_before_probe) { + max_width = dev->mode_config.max_width; + max_height = dev->mode_config.max_width; + }else { + max_width = fb_helper->fb->width; + max_height = fb_helper->fb->height; + }drm_fb_helper_probe_connector_modes (fb_helper, max_width, max_height); -mutex_unlock (&fb_helper->dev->mode_config.mutex); - -drm_modeset_lock_all (dev);drm_setup_crtcs (fb_helper); + +/* If previously there were no connectors with modes and now we +* found some, create new fb and replace default 1024x768 fb */ +for (i =0 ; i < fb_helper->crtc_count; i++) +if (fb_helper->crtc_info[i].desired_mode) { + modes_after_probe = true; +break ; + } +if (!modes_before_probe && modes_after_probe) +drm_fb_helper_single_fb_probe (fb_helper, + fb_helper->fb->bits_per_pixel); +drm_modeset_unlock_all (dev);drm_fb_helper_set_par (fb_helper->fbdev);
drivers/gpu/drm/i915/intel_fbdev.c +13 -13
@@ -199,6 +199,8 @@ out:return ret; } +static void intel_fbdev_destroy (struct fb_info *info); +static int intelfb_create (struct drm_fb_helper *helper,struct drm_fb_helper_surface_size *sizes) {
@@ -220,6 +222,7 @@ static int intelfb_create(struct drm_fb_helper *helper," releasing it\n" , intel_fb->base.width, intel_fb->base.height, sizes->fb_width, sizes->fb_height); +intel_fbdev_destroy (ifbdev->helper.fbdev);drm_framebuffer_unreference (&intel_fb->base); intel_fb = ifbdev->fb =NULL ; }
@@ -545,12 +548,9 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { .fb_probe = intelfb_create, }; -static void intel_fbdev_destroy (struct drm_device *dev, -struct intel_fbdev *ifbdev) +static void intel_fbdev_destroy (struct fb_info *info) { -if (ifbdev->helper.fbdev) { -struct fb_info *info = ifbdev->helper.fbdev; - +if (info) {unregister_framebuffer (info);iounmap (info->screen_base);if (info->cmap.len)
@@ -558,11 +558,6 @@ static void intel_fbdev_destroy(struct drm_device *dev,framebuffer_release (info); } - -drm_fb_helper_fini (&ifbdev->helper); - -drm_framebuffer_unregister_private (&ifbdev->fb->base); -drm_framebuffer_remove (&ifbdev->fb->base); }/*
@@ -750,13 +745,18 @@ void intel_fbdev_initial_config(void *data, async_cookie_t cookie)void intel_fbdev_fini (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; -if (!dev_priv->fbdev) +struct intel_fbdev *ifbdev = dev_priv->fbdev; + +if (!ifbdev)return ;flush_work (&dev_priv->fbdev_suspend_work); -async_synchronize_full (); -intel_fbdev_destroy (dev, dev_priv->fbdev); + +intel_fbdev_destroy (ifbdev->helper.fbdev); +drm_fb_helper_fini (&ifbdev->helper); +drm_framebuffer_unregister_private (&ifbdev->fb->base); +drm_framebuffer_remove (&ifbdev->fb->base);kfree (dev_priv->fbdev); dev_priv->fbdev =NULL ; }
drivers/gpu/drm/nouveau/nouveau_fbcon.c +9 -1
@@ -305,6 +305,9 @@ nouveau_fbcon_zfill(struct drm_device *dev, struct nouveau_fbdev *fbcon) }static int +nouveau_fbcon_destroy (struct drm_device *dev,struct nouveau_fbdev *fbcon); + +static int nouveau_fbcon_create (struct drm_fb_helper *helper,struct drm_fb_helper_surface_size *sizes) {
@@ -322,6 +325,11 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,struct pci_dev *pdev = dev->pdev;int size, ret; +if (helper->fbdev) { +nouveau_fbcon_accel_fini (dev); +nouveau_fbcon_destroy (dev, fbcon); + } + mode_cmd.width = sizes->surface_width; mode_cmd.height = sizes->surface_height;
@@ -467,7 +475,6 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon)drm_gem_object_unreference_unlocked (&nouveau_fb->nvbo->gem); nouveau_fb->nvbo =NULL ; } -drm_fb_helper_fini (&fbcon->helper);drm_framebuffer_unregister_private (&nouveau_fb->base);drm_framebuffer_cleanup (&nouveau_fb->base);return 0 ;
@@ -567,6 +574,7 @@ nouveau_fbcon_fini(struct drm_device *dev)nouveau_fbcon_accel_fini (dev);nouveau_fbcon_destroy (dev, drm->fbcon); +drm_fb_helper_fini (&drm->fbcon->helper);kfree (drm->fbcon); drm->fbcon =NULL ; }
drivers/gpu/drm/radeon/radeon_fb.c +8 -3
@@ -216,6 +216,8 @@ out_unref:return ret; } +static int radeon_fbdev_destroy (struct radeon_fbdev *rfbdev); +static int radeonfb_create (struct drm_fb_helper *helper,struct drm_fb_helper_surface_size *sizes) {
@@ -231,6 +233,9 @@ static int radeonfb_create(struct drm_fb_helper *helper,int ret;unsigned long tmp; +if (helper->fbdev) +radeon_fbdev_destroy (rfbdev); + mode_cmd.width = sizes->surface_width; mode_cmd.height = sizes->surface_height;
@@ -337,7 +342,7 @@ void radeon_fb_output_poll_changed(struct radeon_device *rdev)drm_fb_helper_hotplug_event (&rdev->mode_info.rfbdev->helper); } -static int radeon_fbdev_destroy (struct drm_device *dev,struct radeon_fbdev *rfbdev) +static int radeon_fbdev_destroy (struct radeon_fbdev *rfbdev) {struct fb_info *info;struct radeon_framebuffer *rfb = &rfbdev->rfb;
@@ -355,7 +360,6 @@ static int radeon_fbdev_destroy(struct drm_device *dev, struct radeon_fbdev *rfbradeonfb_destroy_pinned_object (rfb->obj); rfb->obj =NULL ; } -drm_fb_helper_fini (&rfbdev->helper);drm_framebuffer_unregister_private (&rfb->base);drm_framebuffer_cleanup (&rfb->base);
@@ -419,7 +423,8 @@ void radeon_fbdev_fini(struct radeon_device *rdev)if (!rdev->mode_info.rfbdev)return ; -radeon_fbdev_destroy (rdev->ddev, rdev->mode_info.rfbdev); +radeon_fbdev_destroy (rdev->mode_info.rfbdev); +drm_fb_helper_fini (&rdev->mode_info.rfbdev->helper);kfree (rdev->mode_info.rfbdev); rdev->mode_info.rfbdev =NULL ; }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
A message that is not in the archive (sent off-list or not yet received): afe73d5a7382f85c9bdbfc46197a52c4278c99c7.1439288957.git.lukas-JFq808J9C/[email protected]
PATCH v2 17/22 drm/nouveau/timer: Fall back to kernel timer if GPU timer read failed
Unloading the nouveau module while the GPU is asleep (e.g. on dual GPU laptops) leads to an infinite loop in nvkm_timer_wait_eq() because the timer read out is 0xffffffffffffffff so the condition of the while loop becomes -1 - (-1) < nsec and stays like that unless the GPU is woken up.
Use the kernel timer as fallback in this unlikely event. Synchronize the kernel timer and GPU timer in nv04_timer_init() / gk20a_timer_init() which should get called once on driver initialization and on every resume.
Even with this fix applied, unloading the module takes a whopping 167 seconds. This could be reduced by changing the NV_WAIT_DEFAULT timeout from the current (maybe excessive?) 2 seconds to 200 ms.
A WARN_ON is spewed out at nouveau_bo.c:398 after 81 seconds and a null pointer dereference occurs in nouveau_cli_destroy(), so there's more to fix here.
This patch might also be needed to properly handle a GPU connected via Thunderbolt which is suddenly unplugged.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c | 4 ++++ drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c | 9 +++++++++ drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h | 1 + 3 files changed, 14 insertions(+)
drivers/gpu/drm/nouveau/nvkm/subdev/timer/gk20a.c +4 -0
@@ -41,6 +41,10 @@ gk20a_timer_init(struct nvkm_object *object)/* restore the time before suspend */ nv_wr32 (priv,NV04_PTIMER_TIME_1 , hi);nv_wr32 (priv,NV04_PTIMER_TIME_0 , lo); + +/* save kernel time as fallback */ + priv->suspend_ktime =ktime_to_ns (ktime_get ()) - priv->suspend_time; +return 0 ; }
drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.c +9 -0
@@ -36,6 +36,11 @@ nv04_timer_read(struct nvkm_timer *ptimer) lo =nv_rd32 (priv,NV04_PTIMER_TIME_0 ); }while (hi !=nv_rd32 (priv,NV04_PTIMER_TIME_1 )); +if (unlikely (hi ==-1 && lo ==-1 )) { +nv_spam (priv,"read failed, falling back to kernel timer\n" ); +return ktime_to_ns (ktime_get ()) - priv->suspend_ktime; + } +return ((u64 )hi <<32 | lo); }
@@ -216,6 +221,10 @@ nv04_timer_init(struct nvkm_object *object)nv_wr32 (priv,NV04_PTIMER_INTR_EN_0 ,0x00000000 );nv_wr32 (priv,NV04_PTIMER_TIME_1 , hi);nv_wr32 (priv,NV04_PTIMER_TIME_0 , lo); + +/* save kernel time as fallback */ + priv->suspend_ktime =ktime_to_ns (ktime_get ()) - priv->suspend_time; +return 0 ; }
drivers/gpu/drm/nouveau/nvkm/subdev/timer/nv04.h +1 -0
@@ -15,6 +15,7 @@ struct nv04_timer_priv {struct list_head alarms;spinlock_t lock;u64 suspend_time; +u64 suspend_ktime; }; int nv04_timer_ctor(struct nvkm_object *, struct nvkm_object *,
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
Lukas Wunner
reviewerPATCH v2 18/22 vga_switcheroo: Allow using active client as proxy when reading DDC/AUX
The retina MacBook Pro uses an eDP panel and a gmux controller to switch the panel between its two GPUs. Unfortunately it seems that it cannot switch the AUX channel separately from the main link.
But we can emulate switching of DDC/AUX in software by using the active client as a proxy to talk to the panel.
Allow storing pointers to each client's struct i2c_adapter (for DDC) and struct drm_dp_aux. Allow retrieving the active client's structures but constrain access to vga_switcheroo clients to prevent non-clients from using proxying.
Drivers store AUX first, then DDC because they access the DPCD before the EDID. Retrieving AUX is only allowed if DDC is also stored, thereby avoiding race condition where AUX is already stored but not DDC and the inactive client uses AUX then fails on retrieving the EDID via DDC.
Upon storing DDC, generate hotplug event so that already registered inactive clients reprobe once the active client has registered its DDC/AUX structures.
Based (loosely) on patches by Matthew Garrett <[email protected]> who let the active client stash the EDID and the first 8 bytes of the DPCD (receiver capabilities) in vga_switcheroo where the inactive client would subsequently pick it up. It turns out that the drivers are unhappy with just 8 bytes of DPCD, they need access to the full DPCD to set up their outputs. Switching in software gives us more options (even write access to the DPCD if need be):
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0016-vga_switcheroo-Allow-stashing-of-panel-data.patch
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/vga/vga_switcheroo.c | 62 ++++++++++++++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 11 +++++++ 2 files changed, 73 insertions(+)
drivers/gpu/vga/vga_switcheroo.c +62 -0
@@ -19,6 +19,7 @@*/ #include <drm/drm_crtc_helper.h> +#include <drm/drm_dp_helper.h> #include <linux/module.h> #include <linux/seq_file.h>
@@ -27,6 +28,7 @@#include <linux/debugfs.h> #include <linux/fb.h> +#include <linux/i2c.h> #include <linux/pci.h> #include <linux/console.h> #include <linux/vga_switcheroo.h>
@@ -37,6 +39,8 @@struct vga_switcheroo_client {struct pci_dev *pdev;struct fb_info *fb_info; +struct i2c_adapter *ddc; +struct drm_dp_aux *aux;struct work_struct reprobe_work;int pwr_state;const struct vga_switcheroo_client_ops *ops;
@@ -355,6 +359,64 @@ out: }EXPORT_SYMBOL (vga_switcheroo_unlock_ddc); +void vga_switcheroo_set_ddc (struct pci_dev *pdev,struct i2c_adapter *ddc) +{ +struct vga_switcheroo_client *client; + +mutex_lock (&vgasr_mutex); + client =find_client_from_pci (&vgasr_priv.clients, pdev); +if (client) + client->ddc = ddc; +mutex_unlock (&vgasr_mutex); + +/* DDC is stored after AUX on eDP, so we have both now */ +if (client->active) +vga_switcheroo_reprobe_inactive_clients (); +} +EXPORT_SYMBOL (vga_switcheroo_set_ddc); + +struct i2c_adapter *vga_switcheroo_get_ddc (struct pci_dev *pdev) +{ +struct vga_switcheroo_client *active =NULL ; + +mutex_lock (&vgasr_mutex); +if (find_client_from_pci (&vgasr_priv.clients, pdev)) + active =find_active_client (&vgasr_priv.clients); +mutex_unlock (&vgasr_mutex); +if (!active) +return NULL ; + +return active->ddc; +} +EXPORT_SYMBOL (vga_switcheroo_get_ddc); + +void vga_switcheroo_set_aux (struct pci_dev *pdev,struct drm_dp_aux *aux) +{ +struct vga_switcheroo_client *client; + +mutex_lock (&vgasr_mutex); + client =find_client_from_pci (&vgasr_priv.clients, pdev); +if (client) + client->aux = aux; +mutex_unlock (&vgasr_mutex); +} +EXPORT_SYMBOL (vga_switcheroo_set_aux); + +struct drm_dp_aux *vga_switcheroo_get_aux (struct pci_dev *pdev) +{ +struct vga_switcheroo_client *active =NULL ; + +mutex_lock (&vgasr_mutex); +if (find_client_from_pci (&vgasr_priv.clients, pdev)) + active =find_active_client (&vgasr_priv.clients); +mutex_unlock (&vgasr_mutex); +if (!active || !active->ddc) +return NULL ; + +return active->aux; +} +EXPORT_SYMBOL (vga_switcheroo_get_aux); +static int vga_switcheroo_show (struct seq_file *m,void *v) {struct vga_switcheroo_client *client;
include/linux/vga_switcheroo.h +11 -0
@@ -10,7 +10,10 @@#ifndef _LINUX_VGA_SWITCHEROO_H_#define _LINUX_VGA_SWITCHEROO_H_ +#include <drm/drm_dp_helper.h> +#include <linux/fb.h> +#include <linux/i2c.h> struct pci_dev ;
@@ -56,6 +59,10 @@ void vga_switcheroo_client_fb_set(struct pci_dev *dev,int vga_switcheroo_lock_ddc (struct pci_dev *pdev);int vga_switcheroo_unlock_ddc (struct pci_dev *pdev); +void vga_switcheroo_set_ddc (struct pci_dev *pdev,struct i2c_adapter *ddc); +struct i2c_adapter *vga_switcheroo_get_ddc (struct pci_dev *pdev); +void vga_switcheroo_set_aux (struct pci_dev *pdev,struct drm_dp_aux *aux); +struct drm_dp_aux *vga_switcheroo_get_aux (struct pci_dev *pdev);int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler);void vga_switcheroo_unregister_handler (void );
@@ -77,6 +84,10 @@ static inline int vga_switcheroo_register_client(struct pci_dev *dev,static inline void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info) {}static inline int vga_switcheroo_lock_ddc (struct pci_dev *pdev) {return -ENODEV ; }static inline int vga_switcheroo_unlock_ddc (struct pci_dev *pdev) {return -ENODEV ; } +static inline void vga_switcheroo_set_ddc (struct pci_dev *pdev,struct i2c_adapter *ddc) {} +static inline struct i2c_adapter *vga_switcheroo_get_ddc (struct pci_dev *pdev) {return NULL ; } +static inline void vga_switcheroo_set_aux (struct pci_dev *pdev,struct drm_dp_aux *aux) {} +static inline struct drm_dp_aux *vga_switcheroo_get_aux (struct pci_dev *pdev) {return NULL ; }static inline int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler) {return 0 ; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops,
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 19/22 drm: Amend struct drm_dp_aux with connector attribute
On dual GPU laptops which cannot switch the AUX channel separately from the main link and therefore rely on proxying through the vga_switcheroo active client, we need to restrict this to eDP so that only communication to the internal panel is proxied and not to external DP-connected displays which cannot be switched between GPUs, at least on the retina MacBook Pro. Therefore, amend struct drm_dp_aux with a pointer to the connector that the AUX channel belongs to so that we can check the connector type before using proxying.
It actually seems more intuitive that the parent of an AUX channel is a specific connector rather than a device: The device an AUX channel belongs to can be determined from the connector by following connector->dev, but the connector an AUX channel belongs to cannot be unambiguously determined from dev since a device may have multiple DP connectors. Unfortunately we cannot remove the dev attribute since the msm driver calls drm_dp_aux_register way before initializing the connector, and the tegra driver uses a distinct of_device_id for the AUX channel so that it initializes separately from the connector. These two drivers set the newly added connector attribute in struct drm_dp_aux in a delayed fashion, i.e. upon initializing the connector. They are not known to be used for dual GPU laptops right now but maybe they will be in the future, so it seems reasonable to set the attribute in these drivers as well.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/i915/intel_dp.c | 1 + drivers/gpu/drm/msm/edp/edp_connector.c | 2 ++ drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/atombios_dp.c | 1 + drivers/gpu/drm/tegra/sor.c | 1 + include/drm/drm_dp_helper.h | 5 +++++ 6 files changed, 11 insertions(+)
drivers/gpu/drm/i915/intel_dp.c +1 -0
@@ -1066,6 +1066,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) intel_dp->aux.name = name; intel_dp->aux.dev = dev->dev; + intel_dp->aux.connector = &connector->base; intel_dp->aux.transfer = intel_dp_aux_transfer; DRM_DEBUG_KMS("registering %s bus for %s\n", name,
drivers/gpu/drm/msm/edp/edp_connector.c +2 -0
@@ -140,6 +140,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp)drm_connector_helper_add (connector, &edp_connector_helper_funcs); + edp->ctrl->drm_aux->connector = connector; +/* We don't support HPD, so only poll status until connected. */ connector->polled =DRM_CONNECTOR_POLL_CONNECT ;
drivers/gpu/drm/nouveau/nouveau_connector.c +1 -0
@@ -1149,6 +1149,7 @@ nouveau_connector_create(struct drm_device *dev, int index)case DRM_MODE_CONNECTOR_DisplayPort:case DRM_MODE_CONNECTOR_eDP: nv_connector->aux.dev = dev->dev; + nv_connector->aux.connector = connector; nv_connector->aux.transfer = nouveau_connector_aux_xfer; ret =drm_dp_aux_register (&nv_connector->aux);if (ret) {
drivers/gpu/drm/radeon/atombios_dp.c +1 -0
@@ -232,6 +232,7 @@ void radeon_dp_aux_init(struct radeon_connector *radeon_connector) radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd; radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev; + radeon_connector->ddc_bus->aux.connector = &radeon_connector->base;if (ASIC_IS_DCE5 (rdev)) {if (radeon_auxch) radeon_connector->ddc_bus->aux.transfer = radeon_dp_aux_transfer_native;
drivers/gpu/drm/tegra/sor.c +1 -0
@@ -1480,6 +1480,7 @@ static int tegra_sor_init(struct host1x_client *client)drm_connector_helper_add (&sor->output.connector, &tegra_sor_connector_helper_funcs); sor->output.connector.dpms =DRM_MODE_DPMS_OFF ; + sor->dpaux->aux->connector = &sor->output.connector;drm_encoder_init (drm, &sor->output.encoder, &tegra_sor_encoder_funcs,DRM_MODE_ENCODER_TMDS );
include/drm/drm_dp_helper.h +5 -0
@@ -658,6 +658,7 @@ struct drm_dp_aux_msg {* @name: user-visible name of this AUX channel and the I2C-over-AUX adapter * @ddc: I2C adapter that can be used for I2C-over-AUX communication * @dev: pointer to struct device that is the parent for this AUX channel +* @connector: pointer to connector that is the parent for this AUX channel * @hw_mutex: internal mutex used for locking transfers * @transfer: transfers a message representing a single AUX transaction *
@@ -667,6 +668,9 @@ struct drm_dp_aux_msg {* The .name field may be used to specify the name of the I2C adapter. If set to * NULL, dev_name() of .dev will be used. * +* The .connector field should be set to a pointer to the connector that +* the AUX channel belongs to. +* * Drivers provide a hardware-specific implementation of how transactions * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg * structure describing the transaction is passed into this function. Upon
@@ -694,6 +698,7 @@ struct drm_dp_aux {const char *name;struct i2c_adapter ddc;struct device *dev; +struct drm_connector *connector;struct mutex hw_mutex;ssize_t (*transfer)(struct drm_dp_aux *aux,struct drm_dp_aux_msg *msg);
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 20/22 drm: Use vga_switcheroo active client as proxy when reading DDC/AUX
The retina MacBook Pro uses an eDP panel and a gmux controller to switch the panel between its two GPUs. Unfortunately it seems that it cannot switch the AUX channel separately from the main link.
But we can emulate switching of DDC/AUX in software by using the active client as a proxy to talk to the panel.
When reading the DPCD or EDID, store the struct drm_dp_aux and struct i2c_adapter (for DDC) in vga_switcheroo. Retrieve the active client's structures and use them in lieu of the client's own structures.
To avoid using proxying with external DP-connected displays, this is constrained to eDP connectors and further constrained by vga_switcheroo to its clients.
Based (loosely) on patches by Matthew Garrett <[email protected]> who used stashing instead of proxying and changed each driver rather than the generic helper functions:
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0025-i915-Use-vga_switcheroo-to-obtain-and-stash-EDID-DPC.patch
http://www.codon.org.uk/~mjg59/tmp/retina_patches/0026-nouveau-Use-vga_switcheroo-to-obtain-and-stash-EDID-.patch
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/drm_dp_helper.c | 14 ++++++++++++++ drivers/gpu/drm/drm_edid.c | 12 ++++++++++++ 2 files changed, 26 insertions(+)
drivers/gpu/drm/drm_dp_helper.c +14 -0
@@ -27,6 +27,7 @@#include <linux/errno.h> #include <linux/sched.h> #include <linux/i2c.h> +#include <linux/vga_switcheroo.h> #include <drm/drm_dp_helper.h> #include <drm/drmP.h>
@@ -239,6 +240,19 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,ssize_t drm_dp_dpcd_read (struct drm_dp_aux *aux,unsigned int offset,void *buffer,size_t size) { +struct pci_dev *pdev =to_pci_dev (aux->dev); +struct drm_dp_aux *proxy_aux; + +if (aux->connector && + aux->connector->connector_type == DRM_MODE_CONNECTOR_eDP) { +vga_switcheroo_set_aux (pdev, aux); + proxy_aux =vga_switcheroo_get_aux (pdev); +if (proxy_aux && proxy_aux != aux) { +DRM_DEBUG_KMS ("Using vga_switcheroo active client as proxy\n" ); + aux = proxy_aux; + } + } +return drm_dp_dpcd_access (aux,DP_AUX_NATIVE_READ , offset, buffer, size); }
drivers/gpu/drm/drm_edid.c +12 -0
@@ -1379,6 +1379,18 @@ struct edid *drm_get_edid(struct drm_connector *connector, {struct edid *edid;struct pci_dev *pdev = connector->dev->pdev; +struct i2c_adapter *proxy_ddc; + +if (connector->connector_type ==DRM_MODE_CONNECTOR_LVDS || + connector->connector_type == DRM_MODE_CONNECTOR_eDP) { +vga_switcheroo_set_ddc (pdev, adapter); + proxy_ddc =vga_switcheroo_get_ddc (pdev); +if (proxy_ddc && proxy_ddc != adapter) { +DRM_DEBUG_KMS ("Using vga_switcheroo active client as proxy\n" ); + adapter = proxy_ddc; + pdev =to_pci_dev (proxy_ddc->dev.parent); + } + }vga_switcheroo_lock_ddc (pdev);
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 21/22 drm/nouveau/i2c: Use vga_switcheroo active client as proxy when reading DDC/AUX
The retina MacBook Pro uses an eDP panel and a gmux controller to switch the panel between its two GPUs. Unfortunately it seems that it cannot switch the AUX channel separately from the main link.
But we can emulate switching of DDC/AUX in software by using the active client as a proxy to talk to the panel.
Proxying of the AUX channel is facilitated by way of Thierry Reding's awesome struct drm_dp_aux abstraction (cf. c197db75ff5c, "drm/dp: Add AUX channel infrastructure"). However, as regards usage of struct drm_dp_aux, nouveau is the odd man out: A struct drm_dp_aux is defined as part of struct nouveau_connector but never used. Instead, the AUX channel is accessed directly with nv_rdaux() and nv_wraux(), even in the DRM part of the driver.
To enable proxying in nouveau, inject a pointer to the struct drm_dp_aux from the DRM part of the driver into the struct nvkm_i2c_port. Modify nv_rdaux() to try drm_dp_dpcd_read() first. If that fails, fall back to accessing the AUX channel directly. Enclose in #if IS_ENABLED(CONFIG_DRM _KMS_HELPER) to keep the NVKM part of the driver portable and free of DRM symbols.
Obviously this is a bit of a kludge but it seems there's no elegant way short of factoring all the AUX communication in dport.c / outpdp.c out and into the DRM part of the driver (plus the AUX initialization in VBIOS).
When the driver first initializes the output with nvkm_output_dp_init(), the pointer to the struct drm_dp_aux is not yet injected into the struct nvkm_i2c_port. Thus, if the panel is not switched to the Nvidia GPU, the dpcd attribute of struct nvkm_output_dp can't be filled and the link doesn't get trained. Make up for this by checking the link training status in nouveau_dp_detect() and calling nvkm_output_dp_detect() if the link hasn't been trained yet.
Modify link training itself so that it does not fail when writing to DPCD if the value to be written is identical with what's already configured in DPCD. (Proxying is currently read only for safety reasons.)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h | 1 + drivers/gpu/drm/nouveau/nouveau_connector.c | 4 ++-- drivers/gpu/drm/nouveau/nouveau_dp.c | 20 +++++++++++++++++++ drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c | 6 +++++- drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c | 2 +- drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h | 1 + drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 24 +++++++++++++++++++++++ 7 files changed, 54 insertions(+), 4 deletions(-)
drivers/gpu/drm/nouveau/include/nvkm/subdev/i2c.h +1 -0
@@ -37,6 +37,7 @@ struct nvkm_i2c_port {struct list_head head;u8 index;int aux; +void *drm_dp_aux;const struct nvkm_i2c_func *func; };
drivers/gpu/drm/nouveau/nouveau_connector.c +2 -2
@@ -144,8 +144,8 @@ nouveau_connector_ddc_detect(struct drm_connector *connector) nv_encoder =nouveau_encoder (encoder);if (nv_encoder->dcb->type ==DCB_OUTPUT_DP ) { -int ret =nouveau_dp_detect (nv_encoder); -if (ret ==0 ) + nv_encoder->i2c->drm_dp_aux = &nv_connector->aux; +if (nouveau_dp_detect (nv_encoder) ==0 )break ; }else if (nv_encoder->i2c) {
drivers/gpu/drm/nouveau/nouveau_dp.c +20 -0
@@ -30,6 +30,25 @@#include "nouveau_encoder.h" #include "nouveau_crtc.h" +#include <engine/disp.h> +#include <engine/disp/outpdp.h> + +static void +nouveau_dp_check_link_training (struct nouveau_encoder *nv_encoder) +{ +struct nvkm_disp *disp =nvkm_disp (nv_encoder->i2c); +struct nvkm_output *outp; +struct nvkm_output_dp *outpdp; + +list_for_each_entry (outp, &disp->outp, head) +if (outp->info.index == nv_encoder->dcb->index) +break ; + + outpdp = (struct nvkm_output_dp *)outp; +if (!atomic_read (&outpdp->lt.done)) +nvkm_output_dp_detect (outpdp); +} +static void nouveau_dp_probe_oui (struct drm_device *dev,struct nvkm_i2c_port *auxch,u8 *dpcd)
@@ -85,5 +104,6 @@ nouveau_dp_detect(struct nouveau_encoder *nv_encoder) nv_encoder->dp.link_nr, nv_encoder->dp.link_bw);nouveau_dp_probe_oui (dev, auxch, dpcd); +nouveau_dp_check_link_training (nv_encoder);return 0 ; }
drivers/gpu/drm/nouveau/nvkm/engine/disp/dport.c +5 -1
@@ -61,7 +61,7 @@ dp_set_link_config(struct dp_state *dp) .execute = 1, };u32 lnkcmp; -u8 sink[2 ]; +u8 sink[2 ], sink_rd[2 ];int ret;DBG ("%d lanes at %d KB/s\n" , dp->link_nr, dp->link_bw);
@@ -98,6 +98,10 @@ dp_set_link_config(struct dp_state *dp)if (outp->dpcd[DPCD_RC02 ] &DPCD_RC02_ENHANCED_FRAME_CAP ) sink[1 ] |=DPCD_LC01_ENHANCED_FRAME_EN ; +if (nv_rdaux (outp->base.edid,DPCD_LC00_LINK_BW_SET , sink_rd,2 ) ==0 && +memcmp (sink, sink_rd,2 ) ==0 ) +return 0 ; +return nv_wraux (outp->base.edid,DPCD_LC00_LINK_BW_SET , sink,2 ); }
drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.c +1 -1
@@ -122,7 +122,7 @@ nvkm_output_dp_enable(struct nvkm_output_dp *outp, bool present) } } -static void +void nvkm_output_dp_detect (struct nvkm_output_dp *outp) {struct nvkm_i2c_port *port = outp->base.edid;
drivers/gpu/drm/nouveau/nvkm/engine/disp/outpdp.h +1 -0
@@ -58,4 +58,5 @@ struct nvkm_output_dp_impl { };int nvkm_output_dp_train (struct nvkm_output *,u32 rate,bool wait); +void nvkm_output_dp_detect (struct nvkm_output_dp *);#endif
drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c +24 -0
@@ -23,10 +23,34 @@*/ #include "priv.h" +#if IS_ENABLED (CONFIG_DRM_KMS_HELPER ) +#include <drm/drm_dp_helper.h> +#endif + +static int +drm_rdaux (struct nvkm_i2c_port *port,u32 addr,u8 *data,u8 size) +{ +#if IS_ENABLED (CONFIG_DRM_KMS_HELPER ) +if (port->drm_dp_aux) { +nv_debug (port,"Try reading DPCD with KMS helper: addr=0x%x size=%d\n" , + addr, size); +return !(drm_dp_dpcd_read (port->drm_dp_aux, addr, data, size) + == size); + } +#endif +return -ENODEV ; +} +int nv_rdaux (struct nvkm_i2c_port *port,u32 addr,u8 *data,u8 size) {struct nvkm_i2c *i2c =nvkm_i2c (port); + +if (drm_rdaux (port, addr, data, size) ==0 ) +return 0 ; + +nv_debug (port,"Try reading DPCD directly: addr=0x%x size=%d\n" , + addr, size);if (port->func->aux) {int ret = i2c->acquire (port,0 );if (ret ==0 ) {
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerPATCH v2 22/22 drm/nouveau: Use vga_switcheroo active client as proxy when probing DDC on LVDS
The pre-retina MacBook Pro uses an LVDS panel and a gmux controller to switch the panel between its two GPUs. While the gmux is able to switch the DDC lines, we can also emulate DDC switching in software by using the active client as a proxy to talk to the panel. This gives us two ways to switch, one requiring the apple-gmux driver and the other requiring the active client's driver.
To that end, when probing DDC, if we're unable to talk to the panel ourselves and the connector in question is of type LVDS, try proxying via the vga_switcheroo active client.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
--- drivers/gpu/drm/nouveau/nouveau_connector.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/gpu/drm/nouveau/nouveau_connector.c +6 -0
@@ -149,12 +149,18 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)break ; }else if (nv_encoder->i2c) { +struct i2c_adapter *proxy_ddc;int ret;vga_switcheroo_lock_ddc (dev->pdev); ret =nv_probe_i2c (nv_encoder->i2c,0x50 );vga_switcheroo_unlock_ddc (dev->pdev);if (ret)break ; + proxy_ddc =vga_switcheroo_get_ddc (dev->pdev); +if (nv_encoder->dcb->type ==DCB_OUTPUT_LVDS && + nv_connector->type ==DCB_CONNECTOR_LVDS && + proxy_ddc &&drm_probe_ddc (proxy_ddc)) +break ; } }
Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 12/22] drm/i915: Preserve SSC earlier
In reply to Lukas WunnerQuoted 98 lines · Lukas Wunner
Commit
92122789b2d6("drm/i915: preserve SSC if previously set v3") added code tointel_modeset_gem_initto override the SSC status read from VBT with the SSC status set by BIOS.However,
intel_modeset_gem_initis invoked *after*intel_modeset_init, which callsintel_setup_outputs, which *modifies* SSC status by way ofintel_init_pch_refclk. So unlike advertised,intel_modeset_gem_initdoesn't preserve the SSC status set by BIOS but whateverintel_init_pch_refclkdecided on.This is a problem on dual gpu laptops such as the MacBook Pro which require either a handler to switch DDC lines, or the discrete gpu to proxy DDC/AUX communication: Both the handler and the discrete gpu may initialize after the i915 driver, and consequently, an LVDS connector may initially seem disconnected and the SSC therefore is disabled by
intel_init_pch_refclk, but on reprobe the connector may turn out to be connected and the SSC must then be enabled.Due to
92122789b2d6however, the SSC is not enabled on reprobe since it is assumed BIOS disabled it while in fact it was disabled byintel_init_pch_refclk.Also, because the SSC status is preserved so late, the preserved value only ever gets used on resume but not on panel initialization:
intel_modeset_initcallsintel_init_displaywhich indirectly callsintel_panel_use_sscvia multiple subroutines, *before* the BIOS value overrides the VBT value inintel_modeset_gem_init(intel_panel_use_sscis the sole user ofdev_priv->vbt.lvds_use_ssc).Fix this by moving the code introduced by
92122789b2d6fromintel_modeset_gem_inittointel_modeset_initbefore the invocation ofintel_setup_outputsandintel_init_display.Add a
DRM_DEBUG_KMSas suggested way back by Jani:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.htmlBugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]Fixes 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")Signed-off-by Lukas Wunner <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)drivers/gpu/drm/i915/intel_display.c +18
-4@@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)if (INTEL_INFO (dev)->num_pipes ==0 )return ; +/* +* There may be no VBT; and if the BIOS enabled SSC we can +* just keep using it to avoid unnecessary flicker. Whereas if the +* BIOS isn't using it, don't assume it will work even if the VBT +* indicates as much. +*/ +if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) { +bool bios_lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & +DREF_SSC1_ENABLE ); + +if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) { +DRM_DEBUG_KMS ("SSC %sabled by BIOS, overriding VBT which says %sabled\n" , + bios_lvds_use_ssc ?"en" :"dis" , + dev_priv->vbt.lvds_use_ssc ?"en" :"dis" ); + dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc; + } + } +intel_init_display (dev);intel_init_audio (dev);@@ -15446,7 +15464,6 @@ err:void intel_modeset_gem_init (struct drm_device *dev) { -struct drm_i915_private *dev_priv = dev->dev_private;struct drm_crtc *c;struct drm_i915_gem_object *obj;int ret;@@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)intel_init_gt_powersave (dev);mutex_unlock (&dev->struct_mutex); -/* -* There may be no VBT; and if the BIOS enabled SSC we can -* just keep using it to avoid unnecessary flicker. Whereas if the
drivers/gpu/drm/i915/intel_display.c void intel_modeset_gem_init(struct drm_device *dev) · line 15477
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +0
-7-* BIOS isn't using it, don't assume it will work even if the VBT -* indicates as much. -*/ -if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) - dev_priv->vbt.lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & -DREF_SSC1_ENABLE ); -intel_modeset_init_hw (dev);intel_setup_overlay (dev);
Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).
Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula
maintainerRe: [PATCH v2 12/22] drm/i915: Preserve SSC earlier
Quoted 107 lines · Jesse Barnes
Quoted 106 lines · Lukas Wunner
Commit
92122789b2d6("drm/i915: preserve SSC if previously set v3") added code tointel_modeset_gem_initto override the SSC status read from VBT with the SSC status set by BIOS.However,
intel_modeset_gem_initis invoked *after*intel_modeset_init, which callsintel_setup_outputs, which *modifies* SSC status by way ofintel_init_pch_refclk. So unlike advertised,intel_modeset_gem_initdoesn't preserve the SSC status set by BIOS but whateverintel_init_pch_refclkdecided on.This is a problem on dual gpu laptops such as the MacBook Pro which require either a handler to switch DDC lines, or the discrete gpu to proxy DDC/AUX communication: Both the handler and the discrete gpu may initialize after the i915 driver, and consequently, an LVDS connector may initially seem disconnected and the SSC therefore is disabled by
intel_init_pch_refclk, but on reprobe the connector may turn out to be connected and the SSC must then be enabled.Due to
92122789b2d6however, the SSC is not enabled on reprobe since it is assumed BIOS disabled it while in fact it was disabled byintel_init_pch_refclk.Also, because the SSC status is preserved so late, the preserved value only ever gets used on resume but not on panel initialization:
intel_modeset_initcallsintel_init_displaywhich indirectly callsintel_panel_use_sscvia multiple subroutines, *before* the BIOS value overrides the VBT value inintel_modeset_gem_init(intel_panel_use_sscis the sole user ofdev_priv->vbt.lvds_use_ssc).Fix this by moving the code introduced by
92122789b2d6fromintel_modeset_gem_inittointel_modeset_initbefore the invocation ofintel_setup_outputsandintel_init_display.Add a
DRM_DEBUG_KMSas suggested way back by Jani:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.htmlBugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]Fixes 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")Signed-off-by Lukas Wunner <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)drivers/gpu/drm/i915/intel_display.c +18
-11@@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)if (INTEL_INFO (dev)->num_pipes ==0 )return ; +/* +* There may be no VBT; and if the BIOS enabled SSC we can +* just keep using it to avoid unnecessary flicker. Whereas if the +* BIOS isn't using it, don't assume it will work even if the VBT +* indicates as much. +*/ +if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) { +bool bios_lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & +DREF_SSC1_ENABLE ); + +if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) { +DRM_DEBUG_KMS ("SSC %sabled by BIOS, overriding VBT which says %sabled\n" , + bios_lvds_use_ssc ?"en" :"dis" , + dev_priv->vbt.lvds_use_ssc ?"en" :"dis" ); + dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc; + } + } +intel_init_display (dev);intel_init_audio (dev);@@ -15446,7 +15464,6 @@ err:void intel_modeset_gem_init (struct drm_device *dev) { -struct drm_i915_private *dev_priv = dev->dev_private;struct drm_crtc *c;struct drm_i915_gem_object *obj;int ret;@@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)intel_init_gt_powersave (dev);mutex_unlock (&dev->struct_mutex); -/* -* There may be no VBT; and if the BIOS enabled SSC we can -* just keep using it to avoid unnecessary flicker. Whereas if the -* BIOS isn't using it, don't assume it will work even if the VBT -* indicates as much. -*/ -if (HAS_PCH_IBX (dev) ||HAS_PCH_CPT (dev)) - dev_priv->vbt.lvds_use_ssc = !!(I915_READ (PCH_DREF_CONTROL ) & -DREF_SSC1_ENABLE ); -intel_modeset_init_hw (dev);
Quoted 6 lines
drivers/gpu/drm/i915/intel_display.c void intel_modeset_gem_init(struct drm_device *dev) · line 15477
Quoted 2 lines
drivers/gpu/drm/i915/intel_display.c +0
-0intel_setup_overlay (dev);Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).
Reviewed-by Jesse Barnes <[email protected]>
Pushed this one patch to drm-intel-next-fixes, thanks for the patch and review.
BR,
Jani.
Quoted 7 lines
Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"
In reply to Lukas WunnerQuoted 35 lines · Lukas Wunner
This reverts commit
26814ce68904c9faf977c90edac798156311981f.The helper function is no longer needed after Dave Airlie's rewrite of
vga_switcheroo_switch_ddc(), the commit introducing it was only included because31f23c3d488e("drm/edid: Switch DDC when reading the EDID") does not compile without it.Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]Signed-off-by Lukas Wunner <[email protected]>--- drivers/gpu/vga/vga_switcheroo.c | 14 -------------- include/linux/vga_switcheroo.h | 2 -- 2 files changed, 16 deletions(-)drivers/gpu/vga/vga_switcheroo.c +0
-3@@ -214,20 +214,6 @@ find_active_client(struct list_head *head)return NULL ; } -struct pci_dev *vga_switcheroo_get_active_client (void ) -{ -struct vga_switcheroo_client *client;
drivers/gpu/vga/vga_switcheroo.c find_active_client(struct list_head *head) · line 229
Quoted 10 lines
drivers/gpu/vga/vga_switcheroo.c +0
-10-struct pci_dev *pdev =NULL ; - -mutex_lock (&vgasr_mutex); - client =find_active_client (&vgasr_priv.clients); -if (client) - pdev = client->pdev; -mutex_unlock (&vgasr_mutex); -return pdev; -} -EXPORT_SYMBOL (vga_switcheroo_get_active_client);
you just added this earlier in this very series. Please reorder/squash patches so that this isn't required.
-Daniel
Quoted 31 lines
drivers/gpu/vga/vga_switcheroo.c +0
-1-int vga_switcheroo_get_client_state (struct pci_dev *pdev) {struct vga_switcheroo_client *client;include/linux/vga_switcheroo.h +0
-2@@ -63,7 +63,6 @@ void vga_switcheroo_unregister_handler(void);int vga_switcheroo_process_delayed_switch (void ); -struct pci_dev *vga_switcheroo_get_active_client (void );int vga_switcheroo_get_client_state (struct pci_dev *dev);void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic);@@ -85,7 +84,6 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, int id, bool active) { return 0; }static inline void vga_switcheroo_unregister_handler (void ) {}static inline int vga_switcheroo_process_delayed_switch (void ) {return 0 ; } -static inline struct pci_dev *vga_switcheroo_get_active_client (void ) {return NULL ; }static inline int vga_switcheroo_get_client_state (struct pci_dev *dev) {return VGA_SWITCHEROO_ON ; }static inline void vga_switcheroo_set_dynamic_switch (struct pci_dev *pdev,enum vga_switcheroo_state dynamic) {}Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerRe: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"
Hi Daniel,
thanks for taking a look at the patch set.
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Lukas Wunner
This reverts commit
26814ce68904c9faf977c90edac798156311981f.The helper function is no longer needed after Dave Airlie's rewrite of
vga_switcheroo_switch_ddc(), the commit introducing it was only included because31f23c3d488e("drm/edid: Switch DDC when reading the EDID") does not compile without it.
[...]
Quoted 6 lines
Quoted 6 lines
drivers/gpu/vga/vga_switcheroo.c +0
-0@@ -214,20 +214,6 @@ find_active_client(struct list_head *head)return NULL ; }
Quoted 16 lines
drivers/gpu/vga/vga_switcheroo.c find_active_client(struct list_head *head) · line 229
Quoted 13 lines
drivers/gpu/vga/vga_switcheroo.c +0
-13-struct pci_dev *vga_switcheroo_get_active_client (void ) -{ -struct vga_switcheroo_client *client; -struct pci_dev *pdev =NULL ; - -mutex_lock (&vgasr_mutex); - client =find_active_client (&vgasr_priv.clients); -if (client) - pdev = client->pdev; -mutex_unlock (&vgasr_mutex); -return pdev; -} -EXPORT_SYMBOL (vga_switcheroo_get_active_client);you just added this earlier in this very series. Please reorder/squash patches so that this isn't required.
I would have to squash patches 2, 4 (by Seth Forshee), 5 (by Dave Airlie), 6 and 7 (mine). The work of two of these authors would only be acknowledged in the commit message and the history how the code evolved over the course of 3 years would not be reflected in the git repo.
Are you sure? (y/n)
I deliberately didn't squash to preserve authorship and history but if you're forcing me at point blank I'll do it. ;-)
Context: Seth Forshee of Canonical came up with patches in 2012 which Dave Airlie didn't like. He rewrote them and left them as a WIP in his git repo where I picked them up. Matthew Garrett posted patches of his own last year but since they were based on Seth Forshee's code, they didn't get merged either.
The first MacBooks with dual GPUs were introduced 2008, it's 2015 now and we're still missing support in the mainline kernel. The issue is not so much that GPU switching doesn't work (the screen just turns black) but energy consumption because the discrete GPU is used by default and the integrated GPU isn't turned off.
So, machines with huge marketshare + shoddy dual GPU support for years = problem.
We need to fix this, hence the patch set.
Thanks,
Lukas
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 07/22] Revert "vga_switcheroo: Add helper function to get the active client"
Quoted 36 lines · Lukas Wunner
Hi Daniel,
thanks for taking a look at the patch set.
Quoted 7 lines · Daniel Vetter
Quoted 6 lines · Lukas Wunner
This reverts commit
26814ce68904c9faf977c90edac798156311981f.The helper function is no longer needed after Dave Airlie's rewrite of
vga_switcheroo_switch_ddc(), the commit introducing it was only included because31f23c3d488e("drm/edid: Switch DDC when reading the EDID") does not compile without it.[...]
Quoted 6 lines
Quoted 6 lines
drivers/gpu/vga/vga_switcheroo.c +0
-0@@ -214,20 +214,6 @@ find_active_client(struct list_head *head)return NULL ; }Quoted 16 lines
drivers/gpu/vga/vga_switcheroo.c find_active_client(struct list_head *head) · line 229
Quoted 13 lines
drivers/gpu/vga/vga_switcheroo.c +0
-13-struct pci_dev *vga_switcheroo_get_active_client (void ) -{ -struct vga_switcheroo_client *client; -struct pci_dev *pdev =NULL ; - -mutex_lock (&vgasr_mutex); - client =find_active_client (&vgasr_priv.clients); -if (client) - pdev = client->pdev; -mutex_unlock (&vgasr_mutex); -return pdev; -} -EXPORT_SYMBOL (vga_switcheroo_get_active_client);you just added this earlier in this very series. Please reorder/squash patches so that this isn't required.
Quoted 6 lines
I would have to squash patches 2, 4 (by Seth Forshee), 5 (by Dave Airlie), 6 and 7 (mine). The work of two of these authors would only be acknowledged in the commit message and the history how the code evolved over the course of 3 years would not be reflected in the git repo.
Are you sure? (y/n)
Yes just squash and mention that the patch is based on work from $list_of_other_authors, plus cc them. There's not much point in acknowledging when people write broken patches ;-)
Quoted 15 lines
I deliberately didn't squash to preserve authorship and history but if you're forcing me at point blank I'll do it. ;-)
Context: Seth Forshee of Canonical came up with patches in 2012 which Dave Airlie didn't like. He rewrote them and left them as a WIP in his git repo where I picked them up. Matthew Garrett posted patches of his own last year but since they were based on Seth Forshee's code, they didn't get merged either.
The first MacBooks with dual GPUs were introduced 2008, it's 2015 now and we're still missing support in the mainline kernel. The issue is not so much that GPU switching doesn't work (the screen just turns black) but energy consumption because the discrete GPU is used by default and the integrated GPU isn't turned off.
Quoted 4 lines
So, machines with huge marketshare + shoddy dual GPU support for years = problem.
We need to fix this, hence the patch set.
Apparently not a lot of people bothered yet to polish this, so it can't be that bad. I guess everyone just buys the basic model with intel only. -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 06/22] vga_switcheroo: Lock/unlock DDC lines harder
In reply to Lukas WunnerQuoted 14 lines · Lukas Wunner
Unlock DDC lines if
drm_probe_ddc()fails.If the inactive client registers before the active client then
old_ddc_ownercannot be determined withfind_active_client()(null pointer dereference). Therefore change semantics of the ->switch_ddc handler callback to returnold_ddc_owneron success or a negative int on failure.Use
mutex_trylock(&vgasr_mutex)invga_switcheroo_unlock_ddc()to avoid locking inversion when one client locksvgasr_mutexon enteringvga_switcheroo_lock_ddc()and tries to acquireddc_lockwhile another client is holdingddc_lockand tries to acquirevgasr_mutexon enteringvga_switcheroo_unlock_ddc().
Quoted 4 lines
Lock
ddc_lockin stage2 to avoid race condition where reading the EDID and switching happens simultaneously.Switch DDC lines on MIGD / MDIS commands and on runtime suspend.
So essentially you have
bool locked = mutex_trylock();
/* nothing that looks at locked at all */
if (locked)
mutex_unlock;
This doesn't protect anything at all and makes it look _very_ fishy.
I haven't dug deeper yet.
-Daniel
Quoted 293 lines
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]Signed-off-by Lukas Wunner <[email protected]>--- drivers/gpu/drm/drm_edid.c | 9 ++-- drivers/gpu/vga/vga_switcheroo.c | 110 ++++++++++++++++++++++---------------- drivers/platform/x86/apple-gmux.c | 15 +++++- include/linux/vga_switcheroo.h | 3 +- 4 files changed, 87 insertions(+), 50 deletions(-)drivers/gpu/drm/drm_edid.c +6
-3@@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter) {struct edid *edid; +struct pci_dev *pdev = connector->dev->pdev; -vga_switcheroo_lock_ddc (connector->dev->pdev); +vga_switcheroo_lock_ddc (pdev); -if (!drm_probe_ddc (adapter)) +if (!drm_probe_ddc (adapter)) { +vga_switcheroo_unlock_ddc (pdev);return NULL ; + } edid =drm_do_get_edid (connector, drm_do_probe_ddc_edid, adapter);if (edid)drm_get_displayid (connector, edid); -vga_switcheroo_unlock_ddc (connector->dev->pdev); +vga_switcheroo_unlock_ddc (pdev);return edid; }drivers/gpu/vga/vga_switcheroo.c +65
-45@@ -9,12 +9,13 @@Switcher interface - methods require for ATPX and DCM - switchto - this throws the output MUX switch -- discrete_set_power - sets the power state for the discrete card +- switch_ddc - switch only DDC lines, return old DDC owner (or < 0 on failure) +- power_state - sets the power state for either GPU GPU driver interface - set_gpu_state - this should do the equiv of s/r for the card - this should *not* set the discrete power state -- switch_check - check if the device is in a position to switch now +- can_switch - check if the device is in a position to switch now */ #include <linux/module.h> @@ -59,7 +60,7 @@ struct vgasr_priv {struct vga_switcheroo_handler *handler;struct mutex ddc_lock; -struct pci_dev *old_ddc_owner; +enum vga_switcheroo_client_id old_ddc_owner; };#define ID_BIT_AUDIO 0x100@@ -276,33 +277,24 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);int vga_switcheroo_lock_ddc (struct pci_dev *pdev) { -struct vga_switcheroo_client *client; -int ret =0 ; -int id; +int ret, id;mutex_lock (&vgasr_mutex); -if (!vgasr_priv.handler) { +if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { ret = -ENODEV ; goto out; } -if (vgasr_priv.handler->switch_ddc) { -mutex_lock (&vgasr_priv.ddc_lock); +mutex_lock (&vgasr_priv.ddc_lock); + id = vgasr_priv.handler->get_client_id (pdev); + ret = vgasr_priv.handler->switch_ddc (id); - client =find_active_client (&vgasr_priv.clients); -if (!client) { -mutex_unlock (&vgasr_priv.ddc_lock); - ret = -ENODEV ; - goto out; - } - vgasr_priv.old_ddc_owner = client->pdev; -if (client->pdev == pdev) - goto out; - - id = vgasr_priv.handler->get_client_id (pdev); - ret = vgasr_priv.handler->switch_ddc (id); - } +if (ret <0 ) { +pr_err ("vga_switcheroo: failed to switch DDC lines\n" ); +mutex_unlock (&vgasr_priv.ddc_lock); + }else + vgasr_priv.old_ddc_owner = ret; out:mutex_unlock (&vgasr_mutex);@@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc);int vga_switcheroo_unlock_ddc (struct pci_dev *pdev) { -int ret =0 ; -int id; -mutex_lock (&vgasr_mutex); +int ret, id; +bool vgasr_mutex_acquired =mutex_trylock (&vgasr_mutex); -if (!vgasr_priv.handler) { - ret = -ENODEV ; +if (!mutex_is_locked (&vgasr_priv.ddc_lock)) { + ret = -EINVAL ; goto out; } -if (vgasr_priv.handler->switch_ddc) { -if (vgasr_priv.old_ddc_owner != pdev) { - id = vgasr_priv.handler->get_client_id (vgasr_priv.old_ddc_owner); - ret = vgasr_priv.handler->switch_ddc (id); - } - vgasr_priv.old_ddc_owner =NULL ; +if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { +pr_err ("vga_switcheroo: handler disappeared\n" );mutex_unlock (&vgasr_priv.ddc_lock); + ret = -ENODEV ; + goto out; } + + id = vgasr_priv.handler->get_client_id (pdev); +if (vgasr_priv.old_ddc_owner != id) { + ret = vgasr_priv.handler->switch_ddc (vgasr_priv.old_ddc_owner); +if (ret <0 ) +pr_err ("vga_switcheroo: failed to switch DDC lines\n" ); + }else + ret = vgasr_priv.old_ddc_owner; +mutex_unlock (&vgasr_priv.ddc_lock); + out: -mutex_unlock (&vgasr_mutex); +if (vgasr_mutex_acquired) +mutex_unlock (&vgasr_mutex);return ret; }EXPORT_SYMBOL (vga_switcheroo_unlock_ddc);@@ -433,14 +433,26 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) }if (vgasr_priv.handler->switch_ddc) { +mutex_lock (&vgasr_priv.ddc_lock); ret = vgasr_priv.handler->switch_ddc (new_client->id); -if (ret) +mutex_unlock (&vgasr_priv.ddc_lock); +if (ret <0 ) { +pr_err ("vga_switcheroo: failed to switch DDC lines\n" );return ret; + } } ret = vgasr_priv.handler->switchto (new_client->id); -if (ret) - goto restore_ddc; +if (ret) { +pr_err ("vga_switcheroo: failed to switch to client %d\n" , + new_client->id); + active->active = true; +/* restore DDC lines */ +mutex_lock (&vgasr_priv.ddc_lock); + vgasr_priv.handler->switch_ddc (active->id); +mutex_unlock (&vgasr_priv.ddc_lock); +return ret; + }if (new_client->ops->reprobe) new_client->ops->reprobe (new_client->pdev);@@ -452,14 +464,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) new_client->active = true;return 0 ; - -restore_ddc: -if (vgasr_priv.handler->switch_ddc) { -int id = (new_client->id ==VGA_SWITCHEROO_IGD ) ? -VGA_SWITCHEROO_DIS :VGA_SWITCHEROO_IGD ; - ret = vgasr_priv.handler->switch_ddc (id); - } -return ret; }static bool check_can_switch (void )@@ -561,6 +565,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, vgasr_priv.delayed_switch_active = false;if (just_mux) { +if (vgasr_priv.handler->switch_ddc) { +mutex_lock (&vgasr_priv.ddc_lock); + ret = vgasr_priv.handler->switch_ddc (client_id); +mutex_unlock (&vgasr_priv.ddc_lock); +if (ret <0 ) { +pr_err ("vga_switcheroo: failed to switch DDC lines\n" ); + goto out; + } + } ret = vgasr_priv.handler->switchto (client_id); goto out; }@@ -716,6 +729,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) ret = dev->bus->pm->runtime_suspend (dev);if (ret)return ret; +if (vgasr_priv.handler->switch_ddc) { +mutex_lock (&vgasr_priv.ddc_lock); + ret = vgasr_priv.handler->switch_ddc (VGA_SWITCHEROO_IGD ); +mutex_unlock (&vgasr_priv.ddc_lock); +if (ret <0 ) +pr_err ("vga_switcheroo: failed to switch DDC lines\n" ); + }if (vgasr_priv.handler->switchto) vgasr_priv.handler->switchto (VGA_SWITCHEROO_IGD );vga_switcheroo_power_switch (pdev,VGA_SWITCHEROO_OFF );drivers/platform/x86/apple-gmux.c +14
-1@@ -275,11 +275,24 @@ static const struct backlight_ops gmux_bl_ops = {static int gmux_switch_ddc (enum vga_switcheroo_client_id id) { +enum vga_switcheroo_client_id old_ddc_owner; + +if (gmux_read8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ) ==1 ) + old_ddc_owner =VGA_SWITCHEROO_IGD ; +else + old_ddc_owner =VGA_SWITCHEROO_DIS ; + +pr_debug ("Switching gmux DDC from %d to %d\n" , old_ddc_owner, id); + +if (id == old_ddc_owner) +return old_ddc_owner; +if (id ==VGA_SWITCHEROO_IGD )gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,1 );else gmux_write8 (apple_gmux_data,GMUX_PORT_SWITCH_DDC ,2 ); -return 0 ; + +return old_ddc_owner; }static int gmux_switchto (enum vga_switcheroo_client_id id)include/linux/vga_switcheroo.h +2
-1@@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}static inline int vga_switcheroo_register_client (struct pci_dev *dev,const struct vga_switcheroo_client_ops *ops,bool driver_power_control) {return 0 ; }static inline void vga_switcheroo_client_fb_set (struct pci_dev *dev,struct fb_info *info) {} -static inline void vga_switcheroo_switch_ddc (struct pci_dev *pdev) {return NULL ; } +static inline int vga_switcheroo_lock_ddc (struct pci_dev *pdev) {return -ENODEV ; } +static inline int vga_switcheroo_unlock_ddc (struct pci_dev *pdev) {return -ENODEV ; }static inline int vga_switcheroo_register_handler (struct vga_switcheroo_handler *handler) {return 0 ; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops,Signature · 6 lines
1.8.5.2 (Apple Git-48) _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
A message that is not in the archive (sent off-list or not yet received): cover.1439288957.git.lukas-JFq808J9C/[email protected]
Quoted 34 lines · Lukas Wunner
This is a follow-up to the v1 posted in April:
http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.htmlPatches 1 - 17 enable GPU switching on the pre-retina MacBook Pro. These were tested successfully by multiple people and solve two tickets in Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=88861
https://bugs.freedesktop.org/show_bug.cgi?id=61115Patches 18 - 22 are a preview of how we're tackling retina support. Those are marked experimental and are NOT ready to be merged yet. Feedback on them is welcome.
The patches are based on drm-next.
They were tested on the following hardware (thanks a lot everyone!):
Tested-by Paul Hordiienko <[email protected]>[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]Tested-by William Brown <[email protected]>[MBP 8,2 2011 intel SNB + amd turks pre-retina]Tested-by Lukas Wunner <[email protected]>[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]Tested-by Bruno Bierbaumer <[email protected]>[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]What's new:
* By default the MBP boots with the display switched to the discrete GPU but it can be forced to the integrated GPU with an EFI boot variable. Here's a handy tool for that: https://github.com/0xbb/gpu-switch
v1 didn't work in this configuration, v2 does.
Quoted 8 lines
* Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback. v2 simply generates a hotplug event instead. nouveau polls its outputs every 10 seconds so we want it to poll immediately once apple-gmux registers. That is achieved by the hotplug event. The i915 driver is changed to behave identically to nouveau. (Right now it deletes LVDS and eDP connectors from the mode configuration if they can't be probed, deeming them to be ghosts.)
I thought -EDEFERREDPROBE is what we should be using if drivers don't get loaded in the right order? Hand-rolling depency avoidance stuff is imo a horrible idea.
Quoted 6 lines
* Framebuffer recreation if the inactive GPU initializes before the apple-gmux module (i.e. discarding the default 1024x768 fb and replacing with a new one with the actual panel resolution): v1 only supported this for i915, v2 has a generic solution which works with nouveau and radeon as well. (Necessary if the discrete GPU is forced to be the inactive one on boot via the EFI variable.)
Would completely remove the need for this ;-)
Quoted 48 lines
* Generally lots of rough edges were smoothed to hopefully make the patches more suitable for merging. E.g. there's a bug in i915 where the SSC status set by BIOS is preserved too late and v1 only contained a workaround, whereas v2 contains a proper fix in a separate commit.
The long journey towards retina support:
The pixel clock required for retina resolution is not supported by LVDS (which was used on pre-retinas), necessitating eDP instead. Problem is, the gmux register which switches the DDC lines on pre-retinas doesn't switch the AUX channel on retinas. Disassembling the OS X driver revealed that the gmux in retina MBPs gained an additional register
0x7fwhich gets written to when setting up the eDP configuration. There was some hope that this might switch the AUX channel. Alas, we tried writing various values to that register but were unable to get the inactive GPU to talk to the panel. The purpose of register0x7fremains a mystery.Teardowns of the first generation retina MBP name the NXP CBTL06142 and TI HD3SS212 as multiplexers and according to the data sheets I've found, neither supports switching the AUX channel separately from the main link.
Matthew Garrett had the idea of having the active GPU stash the EDID and the first 8 bytes of the DPCD (receiver capabilities) and letting the inactive GPU retrieve that data. I rebased and rewrote his patches and got everything working, only to discover that the drivers are unhappy with just 8 bytes of DPCD. They need full access to the DPCD to set up their outputs. We could stash the entire DPCD but some parts of it are mutable so the stashed data may become stale when the active GPU performs writes to the DPCD.
So I had the idea of using the active GPU as a proxy to talk to the panel, thus emulating switching of the AUX channel in software. We can leverage the struct
drm_dp_auxandi2c_adapter(for DDC) to achieve this, swapping the inactive GPU's structs with those of the active GPU on the fly. That approach is implemented in patches 18 - 22 but there are still some driver issues that I'm debugging. The current status as per the latest logs Bruno sent me is that i915 rejects the mode retrieved via proxying withCLOCK_HIGHand nouveau aborts link training halfway through. Bottom line is that it's not yet working but we're getting closer.As a side effect, the pre-retinas gain a second way to initialize their
outputs: They can either use gmux to switch the DDC lines, or use the active GPU as a proxy for the DDC communication. Which method gets used depends on the order in which the drivers initialize, the inactive GPU will happily use whatever is available and it automatically receives a hotplug event once either method becomes ready for use.
Quoted 4 lines
But, once again, the patches implementing proxying (patches 18 - 22) are still in a state of flux and not ready for prime time, unlike the prior ones which seem stable. Folks are hereby invited to poke holes into them and I'm looking forward to your feedback.
You can't share the dp aux like that. It's true that we need a bit more data (there's a few eDP related feature blocsk we need), but sharing the aux channel entirely is no-go. If you do you get drivers trying to link train and at best this fails and at worst you'll upset the configuration of the other driver and piss of the panel enough to need a hard reset until it works again.
I think just reading edid and the relevant dp aux data in apple-gmux or somewhere like that and stalling driver load until that's ready is the only clean option. And of course we must ensure that inactive drivers don't try to use the epd link at all since that will piss off the panel.
I think the real tricky bit here with vgaswitcheroo is locking, I need to take a separate lock at the patches for that.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
Lukas Wunner
reviewerHi Daniel,
Quoted 12 lines · Daniel Vetter
Quoted 8 lines
* Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback. v2 simply generates a hotplug event instead. nouveau polls its outputs every 10 seconds so we want it to poll immediately once apple-gmux registers. That is achieved by the hotplug event. The i915 driver is changed to behave identically to nouveau. (Right now it deletes LVDS and eDP connectors from the mode configuration if they can't be probed, deeming them to be ghosts.)I thought
-EDEFERREDPROBEis what we should be using if drivers don't get loaded in the right order? Hand-rolling depency avoidance stuff is imo a horrible idea.
[...]
Quoted 3 lines
I think just reading edid and the relevant dp aux data in apple-gmux or somewhere like that and stalling driver load until that's ready is the only clean option.
I'm afraid we can't stall initialization of a driver like that because even though the GPU may not be switched to the panel, it may still have an external monitor attached.
All MacBook Pros have external DP and/or HDMI ports and these are either soldered to the discrete GPU (model year 2011 and onwards) or switchable between the discrete and integrated GPU (until 2010; I think they are even switchable separately from the panel).
So basically we'd have to initialize the driver normally, and if intel_lvds_init() or intel_edp_init_connector() fail we'd have to somehow pass that up the call chain so that i915_pci_probe() can return -EPROBE_DEFER. And whenever we're asked to reprobe we'd repeat initialization of the LVDS or eDP connector.
I'm wondering what the benefit is compared to just keeping the connector in the mode configuration, but with status disconnected, and reprobing it when the ->output_poll_changed callback gets invoked? Because that's what nouveau already does, and what I've changed i915 to do with patch 13.
vga_switcheroo calls drm_kms_helper_hotplug_event() when the handler registers (patch 11), which will invoke ->output_poll_changed. So we're talking about the Official DRM Callback [tm] to probe outputs, not "hand-rolling depency avoidance". :-)
Quoted 8 lines
Quoted 6 lines
* Framebuffer recreation if the inactive GPU initializes before the apple-gmux module (i.e. discarding the default 1024x768 fb and replacing with a new one with the actual panel resolution): v1 only supported this for i915, v2 has a generic solution which works with nouveau and radeon as well. (Necessary if the discrete GPU is forced to be the inactive one on boot via the EFI variable.)
Would completely remove the need for this ;-)
Unfortunately not: We'd still have to initialize the driver to be able to drive external displays. If there are initially no connectors with modes, we'll once again end up with the 1024x768 fb.
Quoted 6 lines
You can't share the dp aux like that. It's true that we need a bit more data (there's a few eDP related feature blocsk we need), but sharing the aux channel entirely is no-go. If you do you get drivers trying to link train and at best this fails and at worst you'll upset the configuration of the other driver and piss of the panel enough to need a hard reset until it works again.
Yes, so far proxying of the AUX channel is read-only. In those cases when writing is necessary for setting up the output, I'm adding code to check if the current content of the DPCD is identical to what's being written and if so, skip the write. We'll see if this stategy is sufficient for the drivers to set up their outputs.
Quoted 2 lines
I think the real tricky bit here with vgaswitcheroo is locking, I need to take a separate lock at the patches for that.
Locking when switching only the DDC lines is facilitated by the ddc_lock attribute of struct vgasr_priv. This is all local to vga_switcheroo.c and contained in patches 5 and 6.
Locking when proxying the AUX channel is facilitated by the hw_mutex attribute of struct drm_dp_aux. nouveau has its own locking mechanism contained in drivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus, when proxying via nouveau, there are two locking mechanisms at work (drm_dp_aux hw_mutex as outer lock + pad as inner lock). This is nothing introduced by this patch set, all existing code.
Locking of access to the struct vgasr_priv is facilitated by the vgasr_mutex in vga_switcheroo.c. Also existing code.
Best regards,
Lukas
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
A message that is not in the archive (sent off-list or not yet received): 20150812233711.GA6002-JFq808J9C/[email protected]
Quoted 41 lines · Lukas Wunner
Hi Daniel,
Quoted 12 lines · Daniel Vetter
Quoted 8 lines
* Reprobing if the inactive GPU initializes before the apple-gmux module:
v1 used Matthew Garrett's approach of adding a driver callback. v2 simply generates a hotplug event instead. nouveau polls its outputs every 10 seconds so we want it to poll immediately once apple-gmux registers. That is achieved by the hotplug event. The i915 driver is changed to behave identically to nouveau. (Right now it deletes LVDS and eDP connectors from the mode configuration if they can't be probed, deeming them to be ghosts.)I thought
-EDEFERREDPROBEis what we should be using if drivers don't get loaded in the right order? Hand-rolling depency avoidance stuff is imo a horrible idea.[...]
Quoted 3 lines
I think just reading edid and the relevant dp aux data in apple-gmux or somewhere like that and stalling driver load until that's ready is the only clean option.
I'm afraid we can't stall initialization of a driver like that because even though the GPU may not be switched to the panel, it may still have an external monitor attached.
All MacBook Pros have external DP and/or HDMI ports and these are either soldered to the discrete GPU (model year 2011 and onwards) or switchable between the discrete and integrated GPU (until 2010; I think they are even switchable separately from the panel).
So basically we'd have to initialize the driver normally, and if
intel_lvds_init()orintel_edp_init_connector()fail we'd have to somehow pass that up the call chain so thati915_pci_probe()can return -EPROBE_DEFER. And whenever we're asked to reprobe we'd repeat initialization of the LVDS or eDP connector.I'm wondering what the benefit is compared to just keeping the connector in the mode configuration, but with status disconnected, and reprobing it when the ->output_poll_changed callback gets invoked? Because that's what nouveau already does, and what I've changed i915 to do with patch 13.
Quoted 4 lines
vga_switcheroocallsdrm_kms_helper_hotplug_event()when the handler registers (patch 11), which will invoke ->output_poll_changed. So we're talking about the Official DRM Callback [tm] to probe outputs, not "hand-rolling depency avoidance". :-)
Oh I didn't spot that one. This kind of layering inversions generally leads to deadlocks and fun stuff. Also reprobing lvds/edp is just a side-effect when you have fbdev emulation enabled. If we go with this re-probing approach then we definitely need a new hook in vga-switcheroo, and even then there's still the locking problem.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
* Framebuffer recreation if the inactive GPU initializes before the apple-gmux module (i.e. discarding the default 1024x768 fb and replacing with a new one with the actual panel resolution): v1 only supported this for i915, v2 has a generic solution which works with nouveau and radeon as well. (Necessary if the discrete GPU is forced to be the inactive one on boot via the EFI variable.)
Quoted 5 lines
Quoted 1 line
Would completely remove the need for this ;-)
Unfortunately not: We'd still have to initialize the driver to be able to drive external displays. If there are initially no connectors with modes, we'll once again end up with the 1024x768 fb.
EDEFERREDPROBE isn't something that gets returned to userspace, it's just internal handling so that the kernel knows there's a depency issue and it needs to retry probing once other drivers have finished loading. It is the generic means linux has to handle cross-driver depencies which aren't reflected in the bus hierarchy.
I.e. it's just something to make sure that apple-gmux is fully loaded before i915/nouveau. The driver _will_ be initialized eventually.
Quoted 7 lines
Quoted 6 lines
You can't share the dp aux like that. It's true that we need a bit more data (there's a few eDP related feature blocsk we need), but sharing the aux channel entirely is no-go. If you do you get drivers trying to link train and at best this fails and at worst you'll upset the configuration of the other driver and piss of the panel enough to need a hard reset until it works again.
Quoted 5 lines
Yes, so far proxying of the AUX channel is read-only. In those cases when writing is necessary for setting up the output, I'm adding code to check if the current content of the DPCD is identical to what's being written and if so, skip the write. We'll see if this stategy is sufficient for the drivers to set up their outputs.
You need to block anything that would write _much_ earlier. By the time we're doing link-training it's way too late.
Quoted 14 lines
Quoted 2 lines
I think the real tricky bit here with vgaswitcheroo is locking, I need to take a separate lock at the patches for that.
Locking when switching only the DDC lines is facilitated by the
ddc_lockattribute of structvgasr_priv. This is all local tovga_switcheroo.cand contained in patches 5 and 6.Locking when proxying the AUX channel is facilitated by the
hw_mutexattribute of structdrm_dp_aux. nouveau has its own locking mechanism contained indrivers/gpu/drm/nouveau/nvkm/subdev/i2c/pad*.c. Thus, when proxying via nouveau, there are two locking mechanisms at work (drm_dp_auxhw_mutexas outer lock + pad as inner lock). This is nothing introduced by this patch set, all existing code.
Quoted 2 lines
Locking of access to the struct
vgasr_privis facilitated by thevgasr_mutexinvga_switcheroo.c. Also existing code.
Making sure everything is protected is the easy part of locking review. Making sure you can't deadlock is the hard part, and the mutex_trylock game looks like that's a problem not handled correctly. -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Nouveau mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/nouveau
Lukas Wunner
reviewerHi Daniel,
Quoted 3 lines · Daniel Vetter
Yes just squash and mention that the patch is based on work from $list_of_other_authors, plus cc them. There's not much point in acknowledging when people write broken patches ;-)
As you requested I've squashed the first 7 patches and I'll post the resulting 3 replacement patches to dri-devel immediately after sending this e-mail.
I've also overhauled locking.
These 3 patches lay the groundwork for DDC switching with gmux. The subsequent patches in the series mostly concern reprobing and though I've made locking-related changes to these as well, I don't want to spam the list with them again until we have reached consensus how reprobing should be implemented:
Quoted 8 lines · Daniel Vetter
EDEFERREDPROBE isn't something that gets returned to userspace, it's just internal handling so that the kernel knows there's a depency issue and it needs to retry probing once other drivers have finished loading. It is the generic means linux has to handle cross-driver depencies which aren't reflected in the bus hierarchy.
I.e. it's just something to make sure that apple-gmux is fully loaded before i915/nouveau. The driver _will_ be initialized eventually.
Aha, so you want to stall initialization of i915/nouveau/radeon *completely* until apple-gmux is loaded.
So how do you determine if initialization should be stalled? You would have to hardcode DMIs for all MacBook Pro models. I just counted it, there are 5 DMIs which require apple-gmux, 2 DMIs which require nouveau and 1 DMI which requires radeon (they require nouveau/radeon for proxying, apple-gmux won't help these models). And every year you would have to add another DMI for the latest MacBook Pro model. People using the latest model with an older kernel won't be able to use switching and may open support tickets.
And if the module required for initialization is not installed or has a different version than the kernel, i915 won't initialize at all, which will be another source for support cases that you'll have to deal with.
I think this doesn't scale and it shows that it's the wrong approach.
vga_switcheroo *knows* when the handler registers, or the driver to proxy AUX, and it can *notify* the inactive GPU's driver. No need to hardcode DMIs, keep it simple.
And there *is* already a callback to notify drivers that they should reprobe their outputs:
* struct drm_mode_config_funcs - basic driver provided mode setting functions
* @output_poll_changed: function to handle output configuration changes
Quoted 6 lines · Lukas Wunner
Quoted 2 lines
vga_switcheroocallsdrm_kms_helper_hotplug_event()when the handler registers (patch 11), which will invoke ->output_poll_changed.Oh I didn't spot that one. This kind of layering inversions generally leads to deadlocks and fun stuff.
Please elaborate why you think it's a layering inversion to call drm_kms_helper_hotplug_event() from vga_switcheroo.
Quoted 2 lines
Also reprobing lvds/edp is just a side-effect when you have fbdev emulation enabled.
No, even though ->output_poll_changed is most commonly used to update the fbcon output configuration, it gets called even if fbdev emulation is disabled, and I've changed i915's callback in patch 13 so that the LVDS/eDP connectors are reprobed even if CONFIG_DRM_I915_FBDEV is not set.
Quoted 2 lines
If we go with this re-probing approach then we definitely need a new hook in vga-switcheroo
Why? From my point of view, drm_kms_helper_hotplug_event() already does the job. The only problem is that i915 removes LVDS and eDP connectors from the mode configuration if they can't be probed. By contrast, nouveau (and I think radeon) will just keep them, but with status disconnected. I changed i915 with patch 13 to behave identically to nouveau/radeon. (Sorry, I've written this before but I feel like I need to overcommunicate in this medium.)
The question is, do you consider it harmful to keep a modeless LVDS or eDP connector in the mode configuration (with status disconnected)? On the MacBooks it works fine but I have no idea if it causes issues on other platforms.
If you absolutely positively believe that this causes issues and don't want to change i915's behaviour, then yes indeed we may need a separate vga_switcheroo callback.
Quoted 1 line
and even then there's still the locking problem.
The only lock held when calling drm_kms_helper_hotplug_event() from vga_switcheroo is vgasr_mutex. When can this deadlock? Whenever we call a vga_switcheroo function from the driver upon probing outputs, specifically:
- vga_switcheroo_client_fb_set()
gets called if the fb is recreated on reprobe
- vga_switcheroo_lock_ddc() / _unlock_ddc()
get called when probing DDC and retrieving the EDID
- vga_switcheroo_set_ddc() / _get_ddc() / _set_aux() / get_aux() get called for proxying
So we can't lock vgasr_mutex in those functions.
In the case of _lock_ddc() / _unlock_ddc() what we're actually protecting against is the sudden deregistration of the handler while we've switched DDC lines. We can solve that by grabbing vgasr_priv.ddc_lock in vga_switcheroo_unregister_handler() to block deregistration until we've switched back. This is what I've done in v2.1 (the 3 patches accompanying this e-mail).
In the case of _fb_set() and the proxying functions, we grab vgasr_mutex because we iterate over the client list and change or retrieve client attributes. What we're actually protecting against is the sudden deregistration of a client while we're working on the client list. We can solve that by introducing a new lock which is grabbed in vga_switcheroo_unregister_client(), in _fb_set() and in the proxying functions.
Best regards,
Lukas
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerHi Dave,
as requested I've pushed the MacBook Pro GPU switching stuff to GitHub to ease browsing/reviewing, latest branch is gmux-v5:
https://github.com/l1k/linux/commits/gmux-v5
(I had applied for a freedesktop.org account in April with bug 89906 but it's still pending, hence GitHub.)
I've overhauled locking once more because previously all EDID reads were serialized even on machines which don't use vga_switcheroo at all. Seems it's impossible to fix this with mutexes and still be race-free and deadlock-free, so I've changed vgasr_mutex to an rwlock:
https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8
There are a number of vga_switcheroo functions added by Takashi Iwai and yourself which don't lock anything at all, I believe this was in part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume which locks vgasr_mutex once again). After changing vgasr_mutex to an rwlock we can safely use locking in those functions as well:
https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62
With this change, on machines which don't use vga_switcheroo, the overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock and EDID reads can happen in parallel. Thierry Reding and Alex Deucher are in favor of adding a separate drm_get_edid_switcheroo() and changing the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu of drm_get_edid() so that drivers which don't use vga_switcheroo avoid the lock_ddc/unlock_ddc calls. Performance-wise these additional calls should be negligible, so I think the motivation can only be better readability/clarity. One should also keep in mind that drivers which don't use vga_switcheroo currently might do so in the future, e.g. if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.
Best regards,
Lukas
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 23 lines · Lukas Wunner
Hi Dave,
as requested I've pushed the MacBook Pro GPU switching stuff to GitHub to ease browsing/reviewing, latest branch is gmux-v5:
https://github.com/l1k/linux/commits/gmux-v5(I had applied for a freedesktop.org account in April with bug 89906 but it's still pending, hence GitHub.)
I've overhauled locking once more because previously all EDID reads were serialized even on machines which don't use
vga_switcherooat all. Seems it's impossible to fix this with mutexes and still be race-free and deadlock-free, so I've changedvgasr_mutexto an rwlock:
https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8There are a number of
vga_switcheroofunctions added by Takashi Iwai and yourself which don't lock anything at all, I believe this was in part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audiolocksvgasr_mutex, wakes up the GPU withvga_switcheroo_runtime_resumewhich locksvgasr_mutexonce again). After changingvgasr_mutexto an rwlock we can safely use locking in those functions as well:
https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62
Quoted 11 lines
With this change, on machines which don't use
vga_switcheroo, the overhead indrm_get_edid()is exactly 1read_lock+ 1read_unlockand EDID reads can happen in parallel. Thierry Reding and Alex Deucher are in favor of adding a separatedrm_get_edid_switcheroo()and changing the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu ofdrm_get_edid()so that drivers which don't usevga_switcherooavoid the lock_ddc/unlock_ddc calls. Performance-wise these additional calls should be negligible, so I think the motivation can only be better readability/clarity. One should also keep in mind that drivers which don't usevga_switcheroocurrently might do so in the future, e.g. if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.
Just a quick aside: Switching to rwlocks to make lockdep happy is a fallacy - lockdep unfortunately doesn't accurately track read lock depencies. Which means very likely you didn't fix the locking inversions but just made lockdep shut up about them. Example:
thread A grabs read-lock 1 and mutex 2.
thread B grabs mutex 2 and write-lock 1.
lockdep won't complain here, but if thread A has tries to get mutex 2 while thread B tries to write-lock 1 then there's an obvious deadlock.
I'd highly suggest you try fixing the vga-switcheroo locking without resorting to rw lock primitives - that just means we need to manually prove the locking for many cases which is tons of work. -Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerHi Daniel,
Quoted 27 lines · Daniel Vetter
Quoted 14 lines · Lukas Wunner
I've overhauled locking once more because previously all EDID reads were serialized even on machines which don't use
vga_switcherooat all. Seems it's impossible to fix this with mutexes and still be race-free and deadlock-free, so I've changedvgasr_mutexto an rwlock:
https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8There are a number of
vga_switcheroofunctions added by Takashi Iwai and yourself which don't lock anything at all, I believe this was in part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audiolocksvgasr_mutex, wakes up the GPU withvga_switcheroo_runtime_resumewhich locksvgasr_mutexonce again). After changingvgasr_mutexto an rwlock we can safely use locking in those functions as well:
https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62Quoted 11 lines
With this change, on machines which don't use
vga_switcheroo, the overhead indrm_get_edid()is exactly 1read_lock+ 1read_unlockand EDID reads can happen in parallel. Thierry Reding and Alex Deucher are in favor of adding a separatedrm_get_edid_switcheroo()and changing the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu ofdrm_get_edid()so that drivers which don't usevga_switcherooavoid the lock_ddc/unlock_ddc calls. Performance-wise these additional calls should be negligible, so I think the motivation can only be better readability/clarity. One should also keep in mind that drivers which don't usevga_switcheroocurrently might do so in the future, e.g. if mobile GPUs use a big.LITTLE concept like ARM CPUs already do.
Quoted 7 lines
Just a quick aside: Switching to rwlocks to make lockdep happy is a fallacy - lockdep unfortunately doesn't accurately track read lock depencies. Which means very likely you didn't fix the locking inversions but just made lockdep shut up about them. [...]
I'd highly suggest you try fixing the vga-switcheroo locking without resorting to rw lock primitives - that just means we need to manually prove the locking for many cases which is tons of work.
mutex is not the right tool for the job:
To make DDC switching bullet proof you need to block the handler from unregistering between lock_ddc and unlock_ddc. Solution: ddc_lock grabs a mutex, ddc_unlock releases it, unregister_handler grabs that same lock.
Downside: All EDID reads are serialized, you can't probe EDID in parallel if you have multiple displays. Which is ugly.
One could be tempted to "solve" this for non-muxed machines by immediately releasing the lock in lock_ddc if there's no handler, and by checking in unlock_ddc if mutex_is_locked before releasing it. But on muxed machines this would be racy: GPU driver A calls lock_ddc, acquires the mutex, sees there's no handler registered, releases the mutex. Between this and the call to unlock_ddc, a handler registers and GPU driver B calls lock_ddc. This time the mutex is actually locked and when driver A calls unlock_ddc, it will release it, even though the other driver had acquired it.
Of course one could come up with all sorts of ugly hacks like remembering for which client the mutex was acquired, but this wouldn't be atomic and 100% bullet proof, unlike rwlocks.
So it seems there's no way with mutexes to achieve parallel EDID reads and still be race-free and deadlock-free.
rwlocks have the right semantics for this use case: Registering and unregistering requires write access to the lock, thus happens exclusively. In lock_ddc we acquire read access to the rwlock and in unlock_ddc we release it. So the handler can't disappear while we've switched DDC. We use a mutex ddc_lock to lock the DDC lines but we only acquire that lock if there's actually a handler. So on non-muxed machines, EDID reads *can* happen in parallel, there's just the (negligible) overhead of 1 read_lock + 1 read_unlock:
https://github.com/l1k/linux/commit/d0b6f659ae8f4b8b94f4b3ded9fc80e93950d0b3
Best regards,
Lukas
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Lukas Wunner
reviewerHi Daniel, Hi Jani,
the patch set I've posted August 12 included 3 commits which fix bugs in i915. These bugs should be fixed independently of MacBook Pro GPU switching, please consider merging them:
drm/i915: Preserve SSC earlier
http://patchwork.freedesktop.org/patch/56921/
drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/
The latter two commits relate to a bug Jani was tracking before his holidays which has unfortunately fallen by the wayside.
Thanks,
Lukas
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula
maintainerQuoted 9 lines · Lukas Wunner
Hi Daniel, Hi Jani,
the patch set I've posted August 12 included 3 commits which fix bugs in i915. These bugs should be fixed independently of MacBook Pro GPU switching, please consider merging them:
drm/i915: Preserve SSC earlier
http://patchwork.freedesktop.org/patch/56921/
Quoted 7 lines
drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/The latter two commits relate to a bug Jani was tracking before his holidays which has unfortunately fallen by the wayside.
Sorry about that. Unfortunately the target is moving fast, and they no longer apply. Please resend on top of current nightly.
BR,
Jani.
Quoted 4 lines
Thanks,
Lukas
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula
maintainerQuoted 9 lines · Jani Nikula
Quoted 8 lines · Lukas Wunner
Hi Daniel, Hi Jani,
the patch set I've posted August 12 included 3 commits which fix bugs in i915. These bugs should be fixed independently of MacBook Pro GPU switching, please consider merging them:
drm/i915: Preserve SSC earlier
http://patchwork.freedesktop.org/patch/56921/
Pushed this one.
Jani.
Quoted 23 lines
Quoted 8 lines
drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/The latter two commits relate to a bug Jani was tracking before his holidays which has unfortunately fallen by the wayside.
Sorry about that. Unfortunately the target is moving fast, and they no longer apply. Please resend on top of current nightly.
BR,
Jani.Quoted 4 lines
Thanks,
Lukas
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Lukas Wunner
reviewerHi Jani,
Quoted 10 lines · Jani Nikula
Quoted 8 lines · Lukas Wunner
the patch set I've posted August 12 included 3 commits which fix bugs in i915. These bugs should be fixed independently of MacBook Pro GPU switching, please consider merging them:
[...]
drm/i915: Fix failure paths around initial fbdev allocation
http://patchwork.freedesktop.org/patch/53673/
drm/i915: On fb alloc failure, unref gem object where it gets refed
http://patchwork.freedesktop.org/patch/53674/
Quoted 2 lines
Sorry about that. Unfortunately the target is moving fast, and they no longer apply. Please resend on top of current nightly.
Alright, coming up in separate e-mails are the above 2 patches rebased on drm-intel-nightly as of this morning. I didn't have to make any changes to the code, if they didn't apply cleanly to your tree it was probably just because of changed diff context.
To ease reviewing I've also pushed them to GitHub:
https://github.com/l1k/linux/commit/f0cd66427039ce1bdc61460a9d833e6d858cff3e
https://github.com/l1k/linux/commit/521e48fc5fc8d211ed2847070120ff4032b7a383
Briefly, the story of the 2 patches is this:
- I had originally reported the issue on June 3:
http://lists.freedesktop.org/archives/intel-gfx/2015-June/067965.html
- Tvrtko came up with a patch which I've tested successfully:
https://patchwork.freedesktop.org/patch/53207/
- However Ville responded to Tvrtko's patch: "I find it rather unexpected that the function drops the passed reference on error. My usual rule is:
do nothing on error, if possible." (see comment section of patchwork link) Tvrtko answered that he didn't have time to look into this further and I wanted it fixed, so I submitted a set of 2 patches, consisting of an adjusted version of Tvrtko's patch, plus another one by me to address Ville's remarks.
So you can either merge Tvrtko's single patch or the 2 patches from me, whichever you prefer. Or request something completely different.
Thanks,
Lukas
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx