From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH RFC] drm/bridge: panel: Add module_get/but calls to attached panel driver Date: Wed, 21 Feb 2018 15:26:14 +0100 Message-ID: <20180221142614.GA30865@ulmo> References: <1519070782-20834-1-git-send-email-jsarha@ti.com> <20180219225923.GG22199@phenom.ffwll.local> <20180220103453.GH23425@ulmo> <864d2538-97ee-3060-efd3-c0accb662e70@ti.com> <20180220120301.GC9556@ulmo> <2a58b75b-98b5-3297-1c7b-78d723f31aae@ti.com> <20180221071842.GA4194@wunner.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0969990282==" Return-path: Received: from mail-qk0-x229.google.com (mail-qk0-x229.google.com [IPv6:2607:f8b0:400d:c09::229]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0936D6E6D6 for ; Wed, 21 Feb 2018 14:26:18 +0000 (UTC) Received: by mail-qk0-x229.google.com with SMTP id l206so2149923qke.1 for ; Wed, 21 Feb 2018 06:26:18 -0800 (PST) In-Reply-To: <20180221071842.GA4194@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: airlied@linux.ie, "Rafael J. Wysocki" , dri-devel@lists.freedesktop.org, tomi.valkeinen@ti.com, Jyri Sarha , laurent.pinchart@ideasonboard.com List-Id: dri-devel@lists.freedesktop.org --===============0969990282== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9amGYk9869ThD9tj" Content-Disposition: inline --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 21, 2018 at 08:18:42AM +0100, Lukas Wunner wrote: > [+cc Rafael] >=20 > On Tue, Feb 20, 2018 at 05:04:00PM +0200, Jyri Sarha wrote: > > On 20/02/18 14:03, Thierry Reding wrote: > > > On Tue, Feb 20, 2018 at 01:28:48PM +0200, Jyri Sarha wrote: > > >> On 20/02/18 12:34, Thierry Reding wrote: > > >>>> On Mon, Feb 19, 2018 at 10:06:22PM +0200, Jyri Sarha wrote: > > >>>>> Currently there is no way for a master drm driver to protect agai= nst an > > >>>>> attached panel driver from being unloaded while it is in use. The > > >>>>> least we can do is to indicate the usage by incrementing the modu= le > > >>>>> reference count. > [...] > > >>> I disagree. module_get() is only going to protect you from unloadin= g a > > >>> module that's in use, but there are other ways to unbind a driver f= rom > > >>> the device and cause subsequent mayhem. > > >>> > > >>> struct device_link was "recently" introduced to fix that issue. > > >> > > >> Unfortunately I do not have time to do full fix for the issue, but in > > >> any case I think this patch is a step to the right direction. The mo= dule > > >> reference count should anyway be increased at least to know that the > > >> driver code remains in memory, even if the device is not there any m= ore. > > >=20 > > > I think device links are actually a lot easier to use and give you a > > > full solution for this. Essentially the only thing you need to do is = add > > > a call to device_link_add() in the correct place. > > >=20 > > > That said, it seems to me like drm_panel_bridge_add() is not a good > > > place to add this, because the panel/bridge does not follow a typical > > > consumer/supplier model. It is rather a helper construct with a well- > > > defined lifetime. > > >=20 > > > What you do want to protect is the panel (the "parent" of the panel/ > > > bridge) from going away unexpectedly. I think the best way to achieve > > > that is to make the link in drm_panel_attach() with something like > > > this: > > >=20 > > > u32 flags =3D DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE; > > > struct device *consumer =3D connector->dev->dev; > > >=20 > > > link =3D device_link_add(consumer, panel->dev, flags); > > > if (!link) { > > > dev_err(panel->dev, "failed to link panel %s to %s\n", > > > dev_name(panel->dev), dev_name(consumer)); > > > return -EINVAL; > > > } > > >=20 > > > Ideally I think we'd link the panel to the connector rather than the > > > top-level DRM device, but we don't have a struct device * for that, so > > > this is probably as good as it gets for now. > >=20 > > Thanks for the hint. Adding the link indeed unbinds the master drm > > device when the panel is unloaded without any complaints. > >=20 > > The annoying thing is that the master drm device does not probe again > > and bind when the supplier device becomes available again. However, > > forcing a manual bind brings the whole device back without a problem. > >=20 > > Is there any trivial way to fix this? >=20 > How about the below, would this work for you? >=20 > Or is the device link added in the consumer's driver, hence is gone when > the consumer is unbound? If so, it should be added somewhere else, > or it shouldn't be removed on consumer unbind. >=20 > @Thierry: Thanks for shifting the discussion in the right direction, > it's good to see device links getting more adoption, instead of > proliferating yet more kludges. However I don't understand why you say > we don't have a struct device for the connector, drm_sysfs_connector_add() > does create one. The problem is that the device created by drm_sysfs_connector_add() is created purely to represent the device in sysfs. That means it won't have a driver bound to it, so if you add the device link to that, then the driver core will try to unbind the sysfs device which isn't bound in the first place. I think what we'd need is for drivers to be able to set that device to whatever their parent is. For example, if there's an IP block on an SoC that provides the connector, then the corresponding pdev->dev should be that pointer. That way the device link would work as expected and tear down the driver for that IP block (hopefully together with the rest of the DRM device). However, doing this via the DRM device's ->dev should also work because a) not all connectors have a parent device and b) the dependency really is from the DRM device to the panel. Thierry --9amGYk9869ThD9tj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlqNgYIACgkQ3SOs138+ s6EtqBAAjnCE/E34LZQE74ceIoacMu3onubzdTtyK44gM9Juw5bCZDcLLN87mflV sn0qqZbyjZ6usf0icrErmWVQwyedOqczzHwYW7YJr43mWS1qVZ2voAxiSNv6vaB1 PLK7fkM/2NAeTlFTUXNusSbHNvkHkh1E2IE2M4zPwYdv7lkFBLOy+pZZdBxncnW3 J4FiQtiHd8mzO4YmRlzb9CcWjgGuUdbNS3MSg24chPav8o7PCHn+eeSjpidQWRH7 J0gpYVKC8XJrk+MWM1bBeUwn01BQh9+j9C13yiEn1jJ/w6RwO57MrTqm/RNI3CQG xMPBBsPGbiK7/TEH3QrIahI6WWU8MqoRSDInNOy6yuAQrF7MFwhA5Eu5NN/vWrtO xbxMMcqcy1cOPMzmmfpXZkHkFgTinkEKa3L4DEIeL6IjmH5GZ0S/mVvkXsne1k8d L/l1VHETZUv32HF1x3EKtvyNp6VvNjQuB+4XwUeAJdYfspI9MXXcCBExgfA0Twmq 1YfjL3C0WT/IPw74d8mkan3hfoRgRnx0RxzzNa5AtatxdTMmiscCre4nGfv6zT3L BPzkT6Y9SsHyWTzFp7rnWysv8ajKKxrCohHhh5+9lbW/SxfgJo8ctTG12xkDJ2Oc tOUWfFfgi9eJu4R8wL0cIBmXX0Cx66zkH1iqw1j5OqAfQBorRFY= =bYUo -----END PGP SIGNATURE----- --9amGYk9869ThD9tj-- --===============0969990282== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0969990282==--