From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6D436C433F5 for ; Thu, 7 Apr 2022 23:46:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232645AbiDGXs2 (ORCPT ); Thu, 7 Apr 2022 19:48:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39428 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232641AbiDGXs2 (ORCPT ); Thu, 7 Apr 2022 19:48:28 -0400 Received: from mail-qt1-x836.google.com (mail-qt1-x836.google.com [IPv6:2607:f8b0:4864:20::836]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F004015448A for ; Thu, 7 Apr 2022 16:46:25 -0700 (PDT) Received: by mail-qt1-x836.google.com with SMTP id z15so243623qtj.13 for ; Thu, 07 Apr 2022 16:46:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MpHTh04rYBwjjTZFAAAByBk97DSNs2GInrUq+lnOv2U=; b=t6TgPFV7nl2Ld7CZKPvMj3/YieaF5IpyvSK7/zJbJsjONfqw8SpTOg0+vhHhuHUGIY A5nIC/iTCgXnf1nuBziDjuSPd+k8tuHxgFb3f5R+Ub9qU5xzJwSnHsMXjNu752fH0GnX 4aVNP6dy7fiAwt/VlzoHLRSWvPvk38NkMdGaEO2fWnfnYnyQLiI+KqSvSSVgrY1srJpv Z0xczrsBRD2mU7P6fU8yd6ZvutfdCYOp8qMt9V9wn+TwlAerWVd2958PFpHHkeiO0eIi TzcJZ7ssXbjOUdgFTDFpnmkJNl5yL0xEUbU/SIl7aRT1nz3Ueyi/vQTwL1iq2MgCGE1w ogJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MpHTh04rYBwjjTZFAAAByBk97DSNs2GInrUq+lnOv2U=; b=n53UJdtfoMVbmZoZJbSj7xW71Era76A08MohXruQiXjpMNzhXzafene5dO0s8Gaug5 HHeWWXSRDdr94wEon44tTUuXaTghEK7DexDx/tybgcz8xSItzbIEt0lGYZ2nh8Z7anaL ZrFbVDWzN+dw8XdlcrbXB7iDI9yM7fHcnF4EcgLnB77vFS2NtU6cEnsb4n9udpu97X88 t17NZlPGVsyT9uHpv+UFru7NZzcS/Qwx1UGDp6QwkQv9RYJ/XpTiZBOV6D2C50oW6fpS eJ3ZXONgJVd05llUJzWXvsU2yluM6bWUyX8sBs2CWwwGC+M19KV5iQubrE9lSt1C6PNk nl0w== X-Gm-Message-State: AOAM531kDLkuvPN/BZ3Jwu+NrXQWcWa8rt9aGChVIV6+TfKLeZc/vG/E +VKNRncqR7W8WQ6Na3Y9QEIdWTQJtuRHKXZd8riJ7A== X-Google-Smtp-Source: ABdhPJzrpa0HTbjgN12dKTJWpgn/QhqrktGsVOt/kJaOnP5/chw8A0IQErROmcv1a6QYCmDVybLZjs+HfF+DkncJGUM= X-Received: by 2002:ac8:4e52:0:b0:2e1:dad8:5141 with SMTP id e18-20020ac84e52000000b002e1dad85141mr13800277qtw.62.1649375185033; Thu, 07 Apr 2022 16:46:25 -0700 (PDT) MIME-Version: 1.0 References: <1648656179-10347-1-git-send-email-quic_sbillaka@quicinc.com> <1648656179-10347-2-git-send-email-quic_sbillaka@quicinc.com> <392b933f-760c-3c81-1040-c514045df3da@linaro.org> <3e5fa57f-d636-879a-b98f-77323d07c156@linaro.org> <225d2c0a-42ec-28ad-688c-e7e9e2035ee1@quicinc.com> In-Reply-To: From: Dmitry Baryshkov Date: Fri, 8 Apr 2022 02:46:13 +0300 Message-ID: Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus To: Doug Anderson Cc: Abhinav Kumar , "Sankeerth Billakanti (QUIC)" , quic_kalyant , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , quic_vproddut , David Airlie , linux-arm-msm , "Kuogee Hsieh (QUIC)" , freedreno , dri-devel , "bjorn.andersson@linaro.org" , Sean Paul , "Aravind Venkateswaran (QUIC)" , Stephen Boyd , Sean Paul , LKML Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Fri, 8 Apr 2022 at 02:35, Doug Anderson wrote: > > Hi, > > On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar wrote: > > > > Hi Doug > > > > Thanks for the response, some comments below. > > > > Abhinav > > On 4/7/2022 1:47 PM, Doug Anderson wrote: > > > Hi, > > > > > > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar wrote: > > >> > > >> Hi Doug and Dmitry > > >> > > >> Sorry, but I caught up on this email just now. > > >> > > >> Some comments below. > > >> > > >> Thanks > > >> > > >> Abhinav > > >> On 4/7/2022 10:07 AM, Doug Anderson wrote: > > >>> Hi, > > >>> > > >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC) > > >>> wrote: > > >>>> > > >>>> Hi Dmitry and Doug, > > >>>> > > >>>>> Hi, > > >>>>> > > >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > > >>>>> wrote: > > >>>>>> > > >>>>>> On 05/04/2022 20:02, Doug Anderson wrote: > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > >>>>>>> wrote: > > >>>>>>>>> 3. For DP and eDP HPD means something a little different. > > >>>>>>>>> Essentially there are two concepts: a) is a display physically > > >>>>>>>>> connected and b) is the display powered up and ready. For DP, the > > >>>>>>>>> two are really tied together. From the kernel's point of view you > > >>>>>>>>> never "power down" a DP display and you can't detect that it's > > >>>>>>>>> physically connected until it's ready. Said another way, on you > > >>>>>>>>> tie "is a display there" to the HPD line and the moment a display > > >>>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the > > >>>>>>>>> lowest power state of a display it _won't_ assert its "HPD" > > >>>>>>>>> signal. However, it's still physically present. For eDP you simply > > >>>>>>>>> have to _assume_ it's present without any actual proof since you > > >>>>>>>>> can't get proof until you power it up. Thus for eDP, you report > > >>>>>>>>> that the display is there as soon as we're asked. We can't _talk_ > > >>>>>>>>> to the display yet, though. So in get_modes() we need to be able > > >>>>>>>>> to power the display on enough to talk over the AUX channel to it. > > >>>>>>>>> As part of this, we wait for the signal named "HPD" which really means > > >>>>> "panel finished powering on" in this context. > > >>>>>>>>> > > >>>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and > > >>>>>>>>> clocks running. We only have enough stuff running to do the AUX > > >>>>> transfer. > > >>>>>>>>> We're not clocking out pixels. We haven't fully powered on the > > >>>>>>>>> display. The AUX transfer is designed to be something that can be > > >>>>>>>>> done early _before_ you turn on the display. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> OK, so basically that was a longwinded way of saying: yes, we > > >>>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the > > >>>>>>>>> way to enable. We have to be able to transfer in get_modes(). If > > >>>>>>>>> you think that's helpful I think it'd be a pretty easy patch to > > >>>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if > > >>>>>>>>> you want me to post it up. > > >>>>>>>> > > >>>>>>>> I think it would be a good idea. At least it will allow us to > > >>>>>>>> judge, which is the more correct way. > > >>>>>>> > > >>>>>>> I'm still happy to prototype this, but the more I think about it the > > >>>>>>> more it feels like a workaround for the Qualcomm driver. The eDP > > >>>>>>> panel driver is actually given a pointer to the AUX bus at probe > > >>>>>>> time. It's really weird to say that we can't do a transfer on it > > >>>>>>> yet... As you said, this is a little sideband bus. It should be able > > >>>>>>> to be used without all the full blown infra of the rest of the driver. > > >>>>>> > > >>>>>> Yes, I have that feeling too. However I also have a feeling that just > > >>>>>> powering up the PHY before the bus probe is ... a hack. There are no > > >>>>>> obvious stopgaps for the driver not to power it down later. > > >>>>> > > >> > > >> Lets go back to why we need to power up the PHY before the bus probe. > > >> > > >> We need to power up PHY before bus probe because panel-eDP tries to read > > >> the EDID in probe() for the panel_id. Not get_modes(). > > >> > > >> So doug, I didnt follow your comment that panel-eDP only does EDID read > > >> in get_modes() > > >> > > >> panel_id = drm_edid_get_panel_id(panel->ddc); > > >> if (!panel_id) { > > >> dev_err(dev, "Couldn't identify panel via EDID\n"); > > >> ret = -EIO; > > >> goto exit; > > >> } > > >> > > >> If we do not need this part, we really dont need to power up the PHY > > >> before the probe(). The hack which dmitry was referring to. > > > > > > Right. ...so we _could_ remove the above from the panel-edp probe and > > > defer it to get_modes() and it wouldn't be that hard. ...but: > > > > > > 1. It feels like a hack to work around the Qualcomm driver. The way > > > the AUX bus is designed is that a pointer to the AUX bus is passed to > > > the panel-edp probe. It seems kinda strange to say that the panel > > > isn't allowed to do transfers with the pointer that's passed in. > > > > > > > And thats why to satisfy the requirements of passing an initialized AUX, > > sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is > > initialized which seems reasonable to satisfy the probe() time requirements. > > > > Even if we move to pm_runtime(), yes I agree it will club all the > > resources needed to control AUX in one place but you will still have to > > initialize PHY before probe() under the hood of pm_runtime(). > > > > So how will it help this cause? > > > > We just have to accept that initializing PHY is a requirement to use AUX > > and before calling panel-eDP's probe(), we have to have an initialized AUX. > > > > So we are not working around the driver but just satisfying the hardware > > requirements to be able to satisfy panel-edp's and > > drm_panel_dp_aux_backlight()'s aux bus requirements. > > The way I'm arguing it should work is that: > > 1. A whole bunch of the DP init code should move to the DP driver's > probe function. This includes parsing the DT, acquiring clocks, > getting a handle to our PHY, and IO mapping registers. As far as I > know, there's no reason to wait on all the components being probed in > order to do this stuff. Yes. And that's one of the reasons I tried to stay away from the DP driver. Each time I open the source code, my hands itch to start refactoring the code. > > 2. Once we have done the above things, it should be possible to do AUX > transfers, correct? ...and then we can populate the AUX bus from the > probe function too. No. In the DP case the AUX bus is inaccessible until the dongle is plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden somewhere in that path) eDP needs to be a special case in the probe() function. > > 3. Any other init (setting up pixel clocks) can continue to happen > where it is today. Yes. > > > > > 2. There's a second place where we might do an AUX transfer at probe > > > time which is when we're using the DP AUX backlight. There we call > > > drm_panel_dp_aux_backlight(). Conceivably this too could be deferred > > > until the get_modes(), but now it feels even more like a hack. We're > > > going to be registering the backlight in the first call to > > > get_modes()? That's, ummm, unexpected. We could look at perhaps > > > breaking the "DP AUX backlight" in two parts also, but that gets > > > involved. I think we're supposed to know the number of backlight > > > levels at device init time for backlight devices and we need an AUX > > > transfer to that. > > > > > > > > > > > > > So the answer is that we could probably make it work, but it seems > > > like an uglier solution than just making the Qualcomm driver able to > > > do AUX transfers when it should be able to. > > > > Correct and by delaying the panel-edp's probe(), we are doing exactly that? > > Right. Where you put the probe now makes it work OK from an AUX > transfer point of view and it's probably OK for the short term, but > I'm not 100% convinced it would handle the -EPROBE_DEFER case, though > I haven't actually tested it. > > Imagine this case: > > 1. 100% of your code is built-in to the kernel except for your PWM > driver, which is a module. > > 2. You start booting up. All the DRM components for MSM are finished > and eventually modeset_init() gets called. > > 3. We try to probe the panel. When the panel tries to acquire the PWM > backlight, it finds that the PWM driver hasn't been loaded yet. It > gets back -EPROBE_DEFER which prevents the panel driver from probing. > > The question is: does modeset_init() handle that -EPROBE_DEFER > elegantly? Normally that's something that would only be returned by > probe functions. Maybe this is all handled, though? I definitely > haven't followed enough of the code and haven't tested it myself. It would be handled up to some point. The error would propagate to the msm_drm_init() = msm_drm_bind(), failing the mdss probe() (and putting it to the defer list). However in the dp's error path the driver would destroy the EP device. The kernel would notice this and retry devices from the defer list. We have just sorted this out for the DSI (thank you Maxime, Rob and Angelo for doing this). > > The above is probably not a giant deal, but I think long term it would > be better to be acquiring resources earlier. -- With best wishes Dmitry From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 632B9C433EF for ; Thu, 7 Apr 2022 23:46:29 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0946C10E050; Thu, 7 Apr 2022 23:46:27 +0000 (UTC) Received: from mail-qt1-x82b.google.com (mail-qt1-x82b.google.com [IPv6:2607:f8b0:4864:20::82b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1749D10E7B4 for ; Thu, 7 Apr 2022 23:46:26 +0000 (UTC) Received: by mail-qt1-x82b.google.com with SMTP id v2so9469958qtc.5 for ; Thu, 07 Apr 2022 16:46:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=MpHTh04rYBwjjTZFAAAByBk97DSNs2GInrUq+lnOv2U=; b=t6TgPFV7nl2Ld7CZKPvMj3/YieaF5IpyvSK7/zJbJsjONfqw8SpTOg0+vhHhuHUGIY A5nIC/iTCgXnf1nuBziDjuSPd+k8tuHxgFb3f5R+Ub9qU5xzJwSnHsMXjNu752fH0GnX 4aVNP6dy7fiAwt/VlzoHLRSWvPvk38NkMdGaEO2fWnfnYnyQLiI+KqSvSSVgrY1srJpv Z0xczrsBRD2mU7P6fU8yd6ZvutfdCYOp8qMt9V9wn+TwlAerWVd2958PFpHHkeiO0eIi TzcJZ7ssXbjOUdgFTDFpnmkJNl5yL0xEUbU/SIl7aRT1nz3Ueyi/vQTwL1iq2MgCGE1w ogJw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=MpHTh04rYBwjjTZFAAAByBk97DSNs2GInrUq+lnOv2U=; b=uZZQT2NmuPv4Qty9hlTwABM0+bIHL/rrc7Iwc5Qd4S93oxNwUtG9yQpkzXNPzA1Mep +duBC2i+3ayMOJm2oVYBxHNo2zcWCext5iurWEu+mWPJNgL+WOvml2ytQBTDYBuUf1Jk OXJo+MneCJsVqgOh7Y7InFNUyMoEqs7i44/zawE5yNI90wZd7GdIZImWcCr/uC5lc20d M4RDQ8VDOwJqCT1b40nlhtXYXKsVI+rzxtVQ3jIkKi5fbFJbX2xAy9qcUyH4BEpyTIaQ GyTvcl+tX281ehM+xEsa6TWdLTxSVJL4ytVeLBG47YDlxXEcpteT5CovOQwK6FyfszlA KDKg== X-Gm-Message-State: AOAM531FaRlDRKAG5tmLOiqGtejcuJhKjWb55PYR0+KWyRXS6oSGnrfz YvSgLCp3e2N+qTsJYrB2PWoD6Yr7UkZyLbGNeaWSMg== X-Google-Smtp-Source: ABdhPJzrpa0HTbjgN12dKTJWpgn/QhqrktGsVOt/kJaOnP5/chw8A0IQErROmcv1a6QYCmDVybLZjs+HfF+DkncJGUM= X-Received: by 2002:ac8:4e52:0:b0:2e1:dad8:5141 with SMTP id e18-20020ac84e52000000b002e1dad85141mr13800277qtw.62.1649375185033; Thu, 07 Apr 2022 16:46:25 -0700 (PDT) MIME-Version: 1.0 References: <1648656179-10347-1-git-send-email-quic_sbillaka@quicinc.com> <1648656179-10347-2-git-send-email-quic_sbillaka@quicinc.com> <392b933f-760c-3c81-1040-c514045df3da@linaro.org> <3e5fa57f-d636-879a-b98f-77323d07c156@linaro.org> <225d2c0a-42ec-28ad-688c-e7e9e2035ee1@quicinc.com> In-Reply-To: From: Dmitry Baryshkov Date: Fri, 8 Apr 2022 02:46:13 +0300 Message-ID: Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus To: Doug Anderson Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: quic_kalyant , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "Sankeerth Billakanti \(QUIC\)" , quic_vproddut , David Airlie , linux-arm-msm , Stephen Boyd , Abhinav Kumar , dri-devel , "Kuogee Hsieh \(QUIC\)" , Sean Paul , Sean Paul , "Aravind Venkateswaran \(QUIC\)" , "bjorn.andersson@linaro.org" , freedreno , LKML Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Fri, 8 Apr 2022 at 02:35, Doug Anderson wrote: > > Hi, > > On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar wrote: > > > > Hi Doug > > > > Thanks for the response, some comments below. > > > > Abhinav > > On 4/7/2022 1:47 PM, Doug Anderson wrote: > > > Hi, > > > > > > On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar wrote: > > >> > > >> Hi Doug and Dmitry > > >> > > >> Sorry, but I caught up on this email just now. > > >> > > >> Some comments below. > > >> > > >> Thanks > > >> > > >> Abhinav > > >> On 4/7/2022 10:07 AM, Doug Anderson wrote: > > >>> Hi, > > >>> > > >>> On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC) > > >>> wrote: > > >>>> > > >>>> Hi Dmitry and Doug, > > >>>> > > >>>>> Hi, > > >>>>> > > >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > > >>>>> wrote: > > >>>>>> > > >>>>>> On 05/04/2022 20:02, Doug Anderson wrote: > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > >>>>>>> wrote: > > >>>>>>>>> 3. For DP and eDP HPD means something a little different. > > >>>>>>>>> Essentially there are two concepts: a) is a display physically > > >>>>>>>>> connected and b) is the display powered up and ready. For DP, the > > >>>>>>>>> two are really tied together. From the kernel's point of view you > > >>>>>>>>> never "power down" a DP display and you can't detect that it's > > >>>>>>>>> physically connected until it's ready. Said another way, on you > > >>>>>>>>> tie "is a display there" to the HPD line and the moment a display > > >>>>>>>>> is there it's ready for you to do AUX transfers. For eDP, in the > > >>>>>>>>> lowest power state of a display it _won't_ assert its "HPD" > > >>>>>>>>> signal. However, it's still physically present. For eDP you simply > > >>>>>>>>> have to _assume_ it's present without any actual proof since you > > >>>>>>>>> can't get proof until you power it up. Thus for eDP, you report > > >>>>>>>>> that the display is there as soon as we're asked. We can't _talk_ > > >>>>>>>>> to the display yet, though. So in get_modes() we need to be able > > >>>>>>>>> to power the display on enough to talk over the AUX channel to it. > > >>>>>>>>> As part of this, we wait for the signal named "HPD" which really means > > >>>>> "panel finished powering on" in this context. > > >>>>>>>>> > > >>>>>>>>> NOTE: for aux transfer, we don't have the _display_ pipe and > > >>>>>>>>> clocks running. We only have enough stuff running to do the AUX > > >>>>> transfer. > > >>>>>>>>> We're not clocking out pixels. We haven't fully powered on the > > >>>>>>>>> display. The AUX transfer is designed to be something that can be > > >>>>>>>>> done early _before_ you turn on the display. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> OK, so basically that was a longwinded way of saying: yes, we > > >>>>>>>>> could avoid the AUX transfer in probe, but we can't wait all the > > >>>>>>>>> way to enable. We have to be able to transfer in get_modes(). If > > >>>>>>>>> you think that's helpful I think it'd be a pretty easy patch to > > >>>>>>>>> write even if it would look a tad bit awkward IMO. Let me know if > > >>>>>>>>> you want me to post it up. > > >>>>>>>> > > >>>>>>>> I think it would be a good idea. At least it will allow us to > > >>>>>>>> judge, which is the more correct way. > > >>>>>>> > > >>>>>>> I'm still happy to prototype this, but the more I think about it the > > >>>>>>> more it feels like a workaround for the Qualcomm driver. The eDP > > >>>>>>> panel driver is actually given a pointer to the AUX bus at probe > > >>>>>>> time. It's really weird to say that we can't do a transfer on it > > >>>>>>> yet... As you said, this is a little sideband bus. It should be able > > >>>>>>> to be used without all the full blown infra of the rest of the driver. > > >>>>>> > > >>>>>> Yes, I have that feeling too. However I also have a feeling that just > > >>>>>> powering up the PHY before the bus probe is ... a hack. There are no > > >>>>>> obvious stopgaps for the driver not to power it down later. > > >>>>> > > >> > > >> Lets go back to why we need to power up the PHY before the bus probe. > > >> > > >> We need to power up PHY before bus probe because panel-eDP tries to read > > >> the EDID in probe() for the panel_id. Not get_modes(). > > >> > > >> So doug, I didnt follow your comment that panel-eDP only does EDID read > > >> in get_modes() > > >> > > >> panel_id = drm_edid_get_panel_id(panel->ddc); > > >> if (!panel_id) { > > >> dev_err(dev, "Couldn't identify panel via EDID\n"); > > >> ret = -EIO; > > >> goto exit; > > >> } > > >> > > >> If we do not need this part, we really dont need to power up the PHY > > >> before the probe(). The hack which dmitry was referring to. > > > > > > Right. ...so we _could_ remove the above from the panel-edp probe and > > > defer it to get_modes() and it wouldn't be that hard. ...but: > > > > > > 1. It feels like a hack to work around the Qualcomm driver. The way > > > the AUX bus is designed is that a pointer to the AUX bus is passed to > > > the panel-edp probe. It seems kinda strange to say that the panel > > > isn't allowed to do transfers with the pointer that's passed in. > > > > > > > And thats why to satisfy the requirements of passing an initialized AUX, > > sankeerth is delaying devm_of_dp_aux_populate_ep_devices() till PHY is > > initialized which seems reasonable to satisfy the probe() time requirements. > > > > Even if we move to pm_runtime(), yes I agree it will club all the > > resources needed to control AUX in one place but you will still have to > > initialize PHY before probe() under the hood of pm_runtime(). > > > > So how will it help this cause? > > > > We just have to accept that initializing PHY is a requirement to use AUX > > and before calling panel-eDP's probe(), we have to have an initialized AUX. > > > > So we are not working around the driver but just satisfying the hardware > > requirements to be able to satisfy panel-edp's and > > drm_panel_dp_aux_backlight()'s aux bus requirements. > > The way I'm arguing it should work is that: > > 1. A whole bunch of the DP init code should move to the DP driver's > probe function. This includes parsing the DT, acquiring clocks, > getting a handle to our PHY, and IO mapping registers. As far as I > know, there's no reason to wait on all the components being probed in > order to do this stuff. Yes. And that's one of the reasons I tried to stay away from the DP driver. Each time I open the source code, my hands itch to start refactoring the code. > > 2. Once we have done the above things, it should be possible to do AUX > transfers, correct? ...and then we can populate the AUX bus from the > probe function too. No. In the DP case the AUX bus is inaccessible until the dongle is plugged (see all the HPD handling, phy_init()/phy_power_on() is hidden somewhere in that path) eDP needs to be a special case in the probe() function. > > 3. Any other init (setting up pixel clocks) can continue to happen > where it is today. Yes. > > > > > 2. There's a second place where we might do an AUX transfer at probe > > > time which is when we're using the DP AUX backlight. There we call > > > drm_panel_dp_aux_backlight(). Conceivably this too could be deferred > > > until the get_modes(), but now it feels even more like a hack. We're > > > going to be registering the backlight in the first call to > > > get_modes()? That's, ummm, unexpected. We could look at perhaps > > > breaking the "DP AUX backlight" in two parts also, but that gets > > > involved. I think we're supposed to know the number of backlight > > > levels at device init time for backlight devices and we need an AUX > > > transfer to that. > > > > > > > > > > > > > So the answer is that we could probably make it work, but it seems > > > like an uglier solution than just making the Qualcomm driver able to > > > do AUX transfers when it should be able to. > > > > Correct and by delaying the panel-edp's probe(), we are doing exactly that? > > Right. Where you put the probe now makes it work OK from an AUX > transfer point of view and it's probably OK for the short term, but > I'm not 100% convinced it would handle the -EPROBE_DEFER case, though > I haven't actually tested it. > > Imagine this case: > > 1. 100% of your code is built-in to the kernel except for your PWM > driver, which is a module. > > 2. You start booting up. All the DRM components for MSM are finished > and eventually modeset_init() gets called. > > 3. We try to probe the panel. When the panel tries to acquire the PWM > backlight, it finds that the PWM driver hasn't been loaded yet. It > gets back -EPROBE_DEFER which prevents the panel driver from probing. > > The question is: does modeset_init() handle that -EPROBE_DEFER > elegantly? Normally that's something that would only be returned by > probe functions. Maybe this is all handled, though? I definitely > haven't followed enough of the code and haven't tested it myself. It would be handled up to some point. The error would propagate to the msm_drm_init() = msm_drm_bind(), failing the mdss probe() (and putting it to the defer list). However in the dp's error path the driver would destroy the EP device. The kernel would notice this and retry devices from the defer list. We have just sorted this out for the DSI (thank you Maxime, Rob and Angelo for doing this). > > The above is probably not a giant deal, but I think long term it would > be better to be acquiring resources earlier. -- With best wishes Dmitry