From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC Date: Mon, 17 Aug 2015 12:36:55 +0200 Message-ID: <20150817103653.GB8453@ulmo.nvidia.com> References: <7984d8eacbb0386ca52e3fdf2ad2554dc90ff1fe.1439739853.git.lukas@wunner.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1406455806==" Return-path: Received: from mail-pd0-f178.google.com (mail-pd0-f178.google.com [209.85.192.178]) by gabe.freedesktop.org (Postfix) with ESMTPS id 70BD8720E7 for ; Mon, 17 Aug 2015 03:38:01 -0700 (PDT) Received: by pdrh1 with SMTP id h1so55155854pdr.0 for ; Mon, 17 Aug 2015 03:38:01 -0700 (PDT) In-Reply-To: <7984d8eacbb0386ca52e3fdf2ad2554dc90ff1fe.1439739853.git.lukas@wunner.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Lukas Wunner Cc: Daniel Vetter , Seth Forshee , dri-devel@lists.freedesktop.org, Dave Airlie List-Id: dri-devel@lists.freedesktop.org --===============1406455806== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="LpQ9ahxlCli8rRTG" Content-Disposition: inline --LpQ9ahxlCli8rRTG Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote: > Originally by Seth Forshee , 2012-10-04: > During graphics driver initialization it's 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. >=20 > Modified by Dave Airlie , 2012-12-22: > I can't figure out why I didn't like this, but I rewrote this [...] to > lock/unlock the ddc lines [...]. I think I'd prefer something like that > otherwise the interface got really ugly. >=20 > Modified by Lukas Wunner , 2015-08-14: > Don't grab vgasr_mutex in lock_ddc/unlock_ddc to avoid the following > deadlock scenarios: (a) During switch (with vgasr_mutex already held), > GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex > to lock DDC lines. (b) Likewise during switch, GPU is suspended and > calls cancel_delayed_work_sync to stop output polling, if poll task > is running at this moment we may wait forever for it to finish. > If we don't grab vgasr_mutex the only bad thing that can happen is > that the handler suddenly disappears. So block handler deregistration > until DDC lines are unlocked again. >=20 > WARN_ON_ONCE if unlock_ddc is called without calling lock_ddc first. > 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. Fix bug in stage2 > where no client had the active attribute set if switching failed. > Fix erroneous interface documentation. >=20 > 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. >=20 > v2.1: Overhaul locking, squash commits (requested by Daniel Vetter) >=20 > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=3D88861 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D61115 > Tested-by: Lukas Wunner > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] >=20 > Cc: Seth Forshee > Cc: Dave Airlie > Signed-off-by: Lukas Wunner > --- > drivers/gpu/vga/vga_switcheroo.c | 73 ++++++++++++++++++++++++++++++++++= +++--- > include/linux/vga_switcheroo.h | 6 ++++ > 2 files changed, 74 insertions(+), 5 deletions(-) >=20 > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switc= heroo.c > index 37ac7b5..ac4ac12 100644 > --- a/drivers/gpu/vga/vga_switcheroo.c > +++ b/drivers/gpu/vga/vga_switcheroo.c > @@ -9,12 +9,13 @@ > =20 > 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 > + - power_state - sets the power state for either GPU > =20 > 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 > */ > =20 > #include > @@ -57,6 +58,8 @@ struct vgasr_priv { > struct list_head clients; > =20 > struct vga_switcheroo_handler *handler; > + struct mutex ddc_lock; > + int old_ddc_owner; > }; > =20 > #define ID_BIT_AUDIO 0x100 > @@ -70,6 +73,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_pr= iv *priv); > /* only one switcheroo per system */ > static struct vgasr_priv vgasr_priv =3D { > .clients =3D LIST_HEAD_INIT(vgasr_priv.clients), > + .ddc_lock =3D __MUTEX_INITIALIZER(vgasr_priv.ddc_lock), > }; > =20 > static bool vga_switcheroo_ready(void) > @@ -122,12 +126,14 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler); > void vga_switcheroo_unregister_handler(void) > { > mutex_lock(&vgasr_mutex); > + mutex_lock(&vgasr_priv.ddc_lock); > vgasr_priv.handler =3D NULL; > if (vgasr_priv.active) { > pr_info("vga_switcheroo: disabled\n"); > vga_switcheroo_debugfs_fini(&vgasr_priv); > vgasr_priv.active =3D false; > } > + mutex_unlock(&vgasr_priv.ddc_lock); > mutex_unlock(&vgasr_mutex); > } > EXPORT_SYMBOL(vga_switcheroo_unregister_handler); > @@ -256,6 +262,43 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pd= ev, > } > EXPORT_SYMBOL(vga_switcheroo_client_fb_set); > =20 > +int vga_switcheroo_lock_ddc(struct pci_dev *pdev) > +{ > + int id; > + > + mutex_lock(&vgasr_priv.ddc_lock); > + > + if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) > + return vgasr_priv.old_ddc_owner =3D -ENODEV; I find this very hard to read. Can this be split across two lines? > + > + id =3D vgasr_priv.handler->get_client_id(pdev); > + return vgasr_priv.old_ddc_owner =3D vgasr_priv.handler->switch_ddc(id); This too. I also notice that the only place you call this from doesn't care about the return value, so why even bother returning one? > +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) > +{ > + int ret, id; > + > + if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock))) > + return -EINVAL; > + > + if (vgasr_priv.old_ddc_owner < 0) { > + mutex_unlock(&vgasr_priv.ddc_lock); > + return -ENODEV; > + } > + > + id =3D vgasr_priv.handler->get_client_id(pdev); > + if (vgasr_priv.old_ddc_owner !=3D id) > + ret =3D vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner); > + else > + ret =3D vgasr_priv.old_ddc_owner; > + mutex_unlock(&vgasr_priv.ddc_lock); > + > + return ret; > +} > +EXPORT_SYMBOL(vga_switcheroo_unlock_ddc); Same comment about the return value here. > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switchero= o.h > index b483abd..62f303f 100644 > --- a/include/linux/vga_switcheroo.h > +++ b/include/linux/vga_switcheroo.h > @@ -29,6 +29,7 @@ enum vga_switcheroo_client_id { > }; > =20 > 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,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev= *pdev, > void vga_switcheroo_client_fb_set(struct pci_dev *dev, > struct fb_info *info); > =20 > +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 *handl= er); > void vga_switcheroo_unregister_handler(void); > =20 > @@ -72,6 +76,8 @@ static inline void vga_switcheroo_unregister_client(str= uct 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, str= uct 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) { retu= rn -ENODEV; } If you care about the return value you'll want to return 0 here to make sure kernels without VGA switcheroo support will continue to work properly. Thierry --LpQ9ahxlCli8rRTG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJV0blCAAoJEN0jrNd/PrOhFt4P/2n6P/7dlNZej3yktutQVTVl NzYpcOgtc7JvWNUfPteAOGiZJzpuyRFDuzRQYakSP3RpRP2lHtBk76ikLrYUOQf6 0BQIXJ5vdgniR6jTt3tv8JAR/rv9t6tL2xVK4j5nPOJgagfgWNC184wfk/bgktUr Ska7fwQdCRk39gwe3UN2wBWRWA0pS6D3i3ngvIgmZsStbB5gL6Nlkf6vV/ruEcG1 FKPS0LQ8tLjag4RVCMJ8bj8NNk2KJji8lxbrqcyiDKsgQ/KdF6u3Rv0EDGRVYqFQ JsTg5fADQfTBifxW1mpkT75RKxG0yGAraMAeA845v5NXiXhBRN4mz52B0gmJHGFd NbcQwKhKOrkL1btMFm5ZaLz/rKoXXH+EALOBS2CH28JnpEmoztjv4S7NO1g8p5bO lNBoUpr9W8Z8zL2EOScED365T7w3PF+7AofxI+I7otN05iatJ9x2OX44KD+Zn6LV a/eHYXhiOK5KoMCK7qndq5gy2sojAlOT+vG/Oi+jS4+nbMq8AA4IZlejhx/uBK5p Gh/6PaWp1DyeeHjS1LBiECcoSRzYyWv+GUX3J+aAzuh13vk2kKrApKUW9xq4onf6 XGAdDJi0fXZ5ogGZi+9Z33d5iYAOe53N0GKh7iaIkRI7jvJ1SZIXVCNizKsU912L 8LxIBLartY1pfwGAcf9x =JeYS -----END PGP SIGNATURE----- --LpQ9ahxlCli8rRTG-- --===============1406455806== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1406455806==--