Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752938AbdFVJXr (ORCPT ); Thu, 22 Jun 2017 05:23:47 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:33625 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751163AbdFVJXp (ORCPT ); Thu, 22 Jun 2017 05:23:45 -0400 Date: Thu, 22 Jun 2017 11:23:32 +0200 From: Boris Brezillon To: Archit Taneja , Laurent Pinchart Cc: Benjamin Gaignard , Eric Anholt , Mark Rutland , devicetree@vger.kernel.org, Linux Kernel Mailing List , "dri-devel@lists.freedesktop.org" , Philippe Cornu , Rob Herring , Thierry Reding Subject: Re: [PATCH 1/7] drm/bridge: Support hotplugging panel-bridge. Message-ID: <20170622112332.7ec7b65c@bbrezillon> In-Reply-To: References: <20170615204130.19255-1-eric@anholt.net> <20170615204130.19255-2-eric@anholt.net> <777ee1b1-e5ce-ea29-8a48-f792354a22d1@codeaurora.org> <871sqkouvr.fsf@eliezer.anholt.net> <8e148170-b626-b426-3c94-b93d2746f4ce@codeaurora.org> <871sqe7ei0.fsf@eliezer.anholt.net> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5034 Lines: 119 On Thu, 22 Jun 2017 13:47:43 +0530 Archit Taneja wrote: > On 06/22/2017 01:20 PM, Benjamin Gaignard wrote: > > 2017-06-20 19:31 GMT+02:00 Eric Anholt : > >> Archit Taneja writes: > >> > >>> On 06/16/2017 08:13 PM, Eric Anholt wrote: > >>>> Archit Taneja writes: > >>>> > >>>>> On 06/16/2017 02:11 AM, Eric Anholt wrote: > >>>>>> If the panel-bridge is being set up after the drm_mode_config_reset(), > >>>>>> then the connector's state would never get initialized, and we'd > >>>>>> dereference the NULL in the hotplug path. We also need to register > >>>>>> the connector, so that userspace can get at it. > >>>>>> > >>>>> > >>>>> Shouldn't the KMS driver make sure the panel-bridge is set up before > >>>>> drm_mode_config_reset? Is it the case when we're inserting the > >>>>> panel-bridge driver as a module? > >>>>> > >>>>> > >>>>> All the connectors that have been added are registered automatically > >>>>> when drm_dev_register() is called by the KMS driver. Registering a > >>>>> connector in the middle of setting up our driver is prone to race > >>>>> conditions if the userspace decides to use them immediately. > >>>> > >>>> Yeah, this is fixing initializing panel_bridge at DSI host_attach time, > >>>> which in the case of a panel module that creates the DSI device > >>>> (adv7533-style, like you said I should use as a reference) will be after > >>>> drm_mode_config_reset() and drm_dev_register(). > >>> > >>> Okay. In the case of the msm kms driver, we defer probe until the > >>> adv7533 module is inserted, only then we proceed to drm_mode_config_reset() > >>> and drm_dev_register(). I assumed this was the general practice followed by > >>> most kms drivers. I.,e the kms driver defers probe until all connector > >>> related modules are inserted, and only then proceed to create a drm device. > >> > >> The problem, though, is the panel driver needs the MIPI DSI host to > >> exist to call mipi_dsi_device_register_full() during the probe process. > >> The adv7533 driver gets around this by registering the DSI device in the > >> bridge attach step, but drm_panel doesn't have an attach step. > > I'm not sure how we can get around this. We had discussion about this on irc > recently, but couldn't come up with a good conclusion. We could come up with a > panel_attach() callback to make it similar to bridges, but that's just us avoiding > the real issue. How about making DSI dev registration fully asynchronous, that is, DSI devs declared in the DT under the DSI host node will be registered/attached at probe time, and devs using another control bus (like the adv7533 controller over i2c) will be registered afterwards. That implies moving the drm_brige registration logic outside of the DSI host ->probe() path. The idea would be to check if all devs connected to the DSI bus are ready at dsi_host->attach() time. If they are, we can finally register the XXX -> DSI bridge. If they're not (because some devs connected to the DSI bus have not been probed yet), then we do not register the drm_bridge and wait for the next dsi_host->attach() event. With this solution, I think we can avoid the chicken&egg problem where the adv7533 dev is waiting for the DSI host to be probed to be able to register a DSI dev with mipi_dsi_device_register_full() and the DSI host needs all devs to be registered to not return -EPROBE_DEFER. > > >> > >> Another alternative is my original version of the panel driver that was > >> a mipi_dsi_device driver that registered the panel during the DSI device > >> probe. That's why vc4's panel lookup is during the MIPI DSI attach > >> phase, currently. > > > > This would require you to have a DSI device node in DT, rather than an i2c > node, right? I don't know if we should do that because of a limitation in > our drm_mipi_dsi and drm_panel frameworks. > > Does anyone have better ideas? Well, these mixed-bus cases are really unpractical to represent in the DT (and also not easy to deal with the device-model). I understand that the initial solution was to put the device under the control-bus, and not the data-bus, but then it leads to the current situation where we don't know exactly when things are ready to be used. Theoretically, we should wait for both the i2c and DSI ends to be attached to their respective controller before starting using the device. But that requires representing things with a 3rd DT node aggregating both components: i2c-bus { ... adv7533_i2c@xx { ... adv,adv7533-master = <&adv7533>; }; }; ... dsi-bus { ... adv7533_dsi: adv7533_dsi@xx { ... adv,adv7533-master = <&adv7533>; }; }; adv7533: adv7533 { ... adv,adv7533-dsi = <&adv7533_dsi>; adv,adv7533-i2c = <&adv7533_i2c>; }; I guess this kind of representation has already been discussed and rejected for good reasons (note that we could also use OF graph representation to link adv7533 master and its components).