From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Doug Anderson <dianders@chromium.org> Cc: Sankeerth Billakanti <quic_sbillaka@quicinc.com>, dri-devel <dri-devel@lists.freedesktop.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, freedreno <freedreno@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Rob Clark <robdclark@gmail.com>, Sean Paul <seanpaul@chromium.org>, Stephen Boyd <swboyd@chromium.org>, quic_kalyant <quic_kalyant@quicinc.com>, "Abhinav Kumar (QUIC)" <quic_abhinavk@quicinc.com>, "Kuogee Hsieh (QUIC)" <quic_khsieh@quicinc.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, quic_vproddut <quic_vproddut@quicinc.com>, quic_aravindh@quicinc.com Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus Date: Sat, 2 Apr 2022 23:26:22 +0300 [thread overview] Message-ID: <CAA8EJppt9XONbgtKfmHmN+==QNqiVJeb8GKJFdZm=yyY-tgmHQ@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=W4PYK-t607yjRbfjDjjEZX0KdgHDRukw_vSH8E8EDH6w@mail.gmail.com> On Sat, 2 Apr 2022 at 20:06, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On 01/04/2022 02:22, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > > <quic_sbillaka@quicinc.com> wrote: > > >> > > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > > >> > > >> dp_display->encoder = encoder; > > >> > > >> + ret = dp_display_get_next_bridge(dp_display); > > >> + if (ret) > > >> + return ret; > > > > > > It feels weird to me that this is in a function called "modeset_init", > > > though I certainly don't know the structure of the MSM display code > > > well enough to fully comment. > > > > It's called modeset_init() as it initializes KMS objects used by DP > > driver. We have similar functions for dsi and hdmi > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or > anything. Mostly saying that I wasn't sure that modeset init was the > proper time to populate the aux bus. ...but then again, perhaps it is > given the current structure of this driver? > > > > > My expectation would have been that > > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > > probe routine and then you would have returned -EPROBE_DEFER from your > > > probe if you were unable to find the panel afterwards. > > > > I don't think it's possible to call it from probe() since > > drm_dp_aux_register() is called only from dp_display_bind(). > > The PHY also isn't initialized at that moment, so we can not probe AUX > > devices. > > > > The overall semantics of the AUX bus is not clear to me. > > Typically the bus is populated (and probed) when devices are accessible. > > But for the display related buses this might not be the case. > > In general the AUX bus is modeled much like the i2c bus. You probe the > sub-device when you're able to transfer. Then you can confirm that the > device is actually there and init the device. > > > > For example for the DSI bus we clearly define that DSI transfer are not > > possible before the corresponding bridge's (or panel's) enable call. > > > > Maybe the same approach should be adopted for the AUX bus. This would > > allow us to populate the AUX bus before hardware access is actually > > possible, thus creating all the DRM bridges before the hardware is > > actually up and running. > > So I guess what you're proposing is that you could probe the devices > under the AUX bus and they could acquire resources (and possibly > return EPROBE_DEFER) at a point in time _before_ it's actually > possible to transfer. Then I guess you'd later do the transfer? Exactly. > > I guess conceivably one could re-design the DRM subsystem like that, > but I don't think it's trivial. The problem is that the DRM subsystem is already designed like that. All the bridge chains are static. They are created during the device probe. And the modes are populated later (via the get_modes() callback), after the HPD signal is delivered. For the encoder/bridge chains it is explicitly stated that the display pipe (clocks and timing signals) are not running before bridge's enable() callback or after the disable() callback being called. > Why? I believe that we need to know > things about the panel at probe time. For instance, we need to be able > to populate the panel's modes. As I said, panel modes are not needed at the probe time. The fact that most (if not all) of the panel drivers provide them in the platform data (and thus modes are typically populated at the probe time) comes from the fact that the panel is usually a known static piece of hardware. With the generic edp-panel this is no longer the case. A single device handles a (probed) variety of panels. Compare it with the generic monitor: We have a known bridge (display-connector.c), so the driver can build the display chain. However a set of modes is not known till the actual monitor is plugged into the device. > To get this information we need the > EDID which means we need to be able to do a transfer. If we're using > an AUX backlight we also need to add info about the backlight at probe > time and that also needs the transfer to work. Yes, the backlight is the problem in the suggested design. I'm not sure when panel->backlight has to be populated for things to work. If we can set it after the probe but before calling into mode_set/drm_bridge_chain_enable(), then it should be fine. > So I guess the net result is maybe we should just keep it where it is. > Long term I'd be interested in knowing if there's a reason why we > can't structure the driver so that AUX transfers can happen with less > intertwining with the rest of the code, but that can happen later. I > would expect that you'd basically just need clocks and regulators on > and maybe your PHY on. Ideally with some pm_runtime fun we should be > able to do that independently with anything else the driver needs to > do? Not really. The driver is shared between the DP and eDP. And the DP (well, combo DP+USB-C) part has quite logical expectations that e.g. AUX channel is not up until all negotiations about the USB-C altmodes are done and the HPD event is delivered. This is the source for my suggestion regarding AUX bus rework/redesign. For non-eDP cases the connected device becomes known much later than the dp_parser code runs (and much later than all the bridges are to be instantiated). Another option would be to keep common drm/msm/dp core code and split the actual driver code into two distinct code paths: one supporting DP, another one supporting eDP. I think, up to now we were trying hard to stay away from such a split. -- With best wishes Dmitry
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Doug Anderson <dianders@chromium.org> Cc: quic_kalyant <quic_kalyant@quicinc.com>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, "Abhinav Kumar \(QUIC\)" <quic_abhinavk@quicinc.com>, quic_vproddut <quic_vproddut@quicinc.com>, David Airlie <airlied@linux.ie>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, LKML <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Stephen Boyd <swboyd@chromium.org>, Sean Paul <sean@poorly.run>, Sean Paul <seanpaul@chromium.org>, quic_aravindh@quicinc.com, "Kuogee Hsieh \(QUIC\)" <quic_khsieh@quicinc.com>, freedreno <freedreno@lists.freedesktop.org> Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus Date: Sat, 2 Apr 2022 23:26:22 +0300 [thread overview] Message-ID: <CAA8EJppt9XONbgtKfmHmN+==QNqiVJeb8GKJFdZm=yyY-tgmHQ@mail.gmail.com> (raw) In-Reply-To: <CAD=FV=W4PYK-t607yjRbfjDjjEZX0KdgHDRukw_vSH8E8EDH6w@mail.gmail.com> On Sat, 2 Apr 2022 at 20:06, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On 01/04/2022 02:22, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > > > <quic_sbillaka@quicinc.com> wrote: > > >> > > >> @@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > > >> > > >> dp_display->encoder = encoder; > > >> > > >> + ret = dp_display_get_next_bridge(dp_display); > > >> + if (ret) > > >> + return ret; > > > > > > It feels weird to me that this is in a function called "modeset_init", > > > though I certainly don't know the structure of the MSM display code > > > well enough to fully comment. > > > > It's called modeset_init() as it initializes KMS objects used by DP > > driver. We have similar functions for dsi and hdmi > > Sorry, I wasn't meaning to imply that modeset_init() was a bad name or > anything. Mostly saying that I wasn't sure that modeset init was the > proper time to populate the aux bus. ...but then again, perhaps it is > given the current structure of this driver? > > > > > My expectation would have been that > > > devm_of_dp_aux_populate_ep_devices() would have been called from your > > > probe routine and then you would have returned -EPROBE_DEFER from your > > > probe if you were unable to find the panel afterwards. > > > > I don't think it's possible to call it from probe() since > > drm_dp_aux_register() is called only from dp_display_bind(). > > The PHY also isn't initialized at that moment, so we can not probe AUX > > devices. > > > > The overall semantics of the AUX bus is not clear to me. > > Typically the bus is populated (and probed) when devices are accessible. > > But for the display related buses this might not be the case. > > In general the AUX bus is modeled much like the i2c bus. You probe the > sub-device when you're able to transfer. Then you can confirm that the > device is actually there and init the device. > > > > For example for the DSI bus we clearly define that DSI transfer are not > > possible before the corresponding bridge's (or panel's) enable call. > > > > Maybe the same approach should be adopted for the AUX bus. This would > > allow us to populate the AUX bus before hardware access is actually > > possible, thus creating all the DRM bridges before the hardware is > > actually up and running. > > So I guess what you're proposing is that you could probe the devices > under the AUX bus and they could acquire resources (and possibly > return EPROBE_DEFER) at a point in time _before_ it's actually > possible to transfer. Then I guess you'd later do the transfer? Exactly. > > I guess conceivably one could re-design the DRM subsystem like that, > but I don't think it's trivial. The problem is that the DRM subsystem is already designed like that. All the bridge chains are static. They are created during the device probe. And the modes are populated later (via the get_modes() callback), after the HPD signal is delivered. For the encoder/bridge chains it is explicitly stated that the display pipe (clocks and timing signals) are not running before bridge's enable() callback or after the disable() callback being called. > Why? I believe that we need to know > things about the panel at probe time. For instance, we need to be able > to populate the panel's modes. As I said, panel modes are not needed at the probe time. The fact that most (if not all) of the panel drivers provide them in the platform data (and thus modes are typically populated at the probe time) comes from the fact that the panel is usually a known static piece of hardware. With the generic edp-panel this is no longer the case. A single device handles a (probed) variety of panels. Compare it with the generic monitor: We have a known bridge (display-connector.c), so the driver can build the display chain. However a set of modes is not known till the actual monitor is plugged into the device. > To get this information we need the > EDID which means we need to be able to do a transfer. If we're using > an AUX backlight we also need to add info about the backlight at probe > time and that also needs the transfer to work. Yes, the backlight is the problem in the suggested design. I'm not sure when panel->backlight has to be populated for things to work. If we can set it after the probe but before calling into mode_set/drm_bridge_chain_enable(), then it should be fine. > So I guess the net result is maybe we should just keep it where it is. > Long term I'd be interested in knowing if there's a reason why we > can't structure the driver so that AUX transfers can happen with less > intertwining with the rest of the code, but that can happen later. I > would expect that you'd basically just need clocks and regulators on > and maybe your PHY on. Ideally with some pm_runtime fun we should be > able to do that independently with anything else the driver needs to > do? Not really. The driver is shared between the DP and eDP. And the DP (well, combo DP+USB-C) part has quite logical expectations that e.g. AUX channel is not up until all negotiations about the USB-C altmodes are done and the HPD event is delivered. This is the source for my suggestion regarding AUX bus rework/redesign. For non-eDP cases the connected device becomes known much later than the dp_parser code runs (and much later than all the bridges are to be instantiated). Another option would be to keep common drm/msm/dp core code and split the actual driver code into two distinct code paths: one supporting DP, another one supporting eDP. I think, up to now we were trying hard to stay away from such a split. -- With best wishes Dmitry
next prev parent reply other threads:[~2022-04-02 20:26 UTC|newest] Thread overview: 140+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-30 16:02 [PATCH v6 0/8] Add support for the eDP panel over aux_bus Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-30 16:02 ` [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-30 23:19 ` Dmitry Baryshkov 2022-03-30 23:19 ` Dmitry Baryshkov 2022-03-31 0:33 ` Doug Anderson 2022-03-31 0:33 ` Doug Anderson 2022-03-31 23:22 ` Doug Anderson 2022-03-31 23:22 ` Doug Anderson 2022-04-02 10:37 ` Dmitry Baryshkov 2022-04-02 10:37 ` Dmitry Baryshkov 2022-04-02 17:06 ` Doug Anderson 2022-04-02 17:06 ` Doug Anderson 2022-04-02 20:26 ` Dmitry Baryshkov [this message] 2022-04-02 20:26 ` Dmitry Baryshkov 2022-04-04 20:53 ` Doug Anderson 2022-04-04 20:53 ` Doug Anderson 2022-04-05 12:53 ` Dmitry Baryshkov 2022-04-05 12:53 ` Dmitry Baryshkov 2022-04-05 17:02 ` Doug Anderson 2022-04-05 17:02 ` Doug Anderson 2022-04-05 17:36 ` Dmitry Baryshkov 2022-04-05 17:36 ` Dmitry Baryshkov 2022-04-05 18:11 ` Doug Anderson 2022-04-05 18:11 ` Doug Anderson 2022-04-07 14:19 ` Sankeerth Billakanti (QUIC) 2022-04-07 14:19 ` Sankeerth Billakanti (QUIC) 2022-04-07 17:07 ` Doug Anderson 2022-04-07 17:07 ` Doug Anderson 2022-04-07 20:11 ` Abhinav Kumar 2022-04-07 20:11 ` Abhinav Kumar 2022-04-07 20:47 ` Doug Anderson 2022-04-07 20:47 ` Doug Anderson 2022-04-07 22:03 ` Abhinav Kumar 2022-04-07 22:03 ` Abhinav Kumar 2022-04-07 23:34 ` Doug Anderson 2022-04-07 23:34 ` Doug Anderson 2022-04-07 23:46 ` Dmitry Baryshkov 2022-04-07 23:46 ` Dmitry Baryshkov 2022-04-08 0:21 ` Doug Anderson 2022-04-08 0:21 ` Doug Anderson 2022-04-08 12:19 ` Dmitry Baryshkov 2022-04-08 12:19 ` Dmitry Baryshkov 2022-04-08 13:43 ` Doug Anderson 2022-04-08 13:43 ` Doug Anderson 2022-04-08 14:58 ` Dmitry Baryshkov 2022-04-08 14:58 ` Dmitry Baryshkov 2022-04-08 17:23 ` Abhinav Kumar 2022-04-08 17:23 ` Abhinav Kumar 2022-04-07 23:35 ` Dmitry Baryshkov 2022-04-07 23:35 ` Dmitry Baryshkov 2022-04-08 0:20 ` Doug Anderson 2022-04-08 0:20 ` Doug Anderson 2022-04-08 12:13 ` Dmitry Baryshkov 2022-04-08 12:13 ` Dmitry Baryshkov 2022-04-08 13:56 ` Doug Anderson 2022-04-08 13:56 ` Doug Anderson 2022-04-08 14:17 ` Dmitry Baryshkov 2022-04-08 14:17 ` Dmitry Baryshkov 2022-03-30 16:02 ` [PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-31 23:22 ` Doug Anderson 2022-03-31 23:22 ` Doug Anderson 2022-04-04 12:43 ` Sankeerth Billakanti (QUIC) 2022-04-04 12:43 ` Sankeerth Billakanti (QUIC) 2022-03-30 16:02 ` [PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-31 23:22 ` Doug Anderson 2022-03-31 23:22 ` Doug Anderson 2022-04-04 12:56 ` Sankeerth Billakanti (QUIC) 2022-04-04 12:56 ` Sankeerth Billakanti (QUIC) 2022-03-30 16:02 ` [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-30 22:16 ` Dmitry Baryshkov 2022-03-30 22:16 ` Dmitry Baryshkov 2022-03-31 5:53 ` Sankeerth Billakanti (QUIC) 2022-03-31 5:53 ` Sankeerth Billakanti (QUIC) 2022-03-31 10:10 ` Dmitry Baryshkov 2022-03-31 10:10 ` Dmitry Baryshkov 2022-03-31 11:04 ` Sankeerth Billakanti 2022-03-31 11:04 ` Sankeerth Billakanti 2022-03-31 11:06 ` Dmitry Baryshkov 2022-03-31 11:06 ` Dmitry Baryshkov 2022-04-04 17:56 ` Sankeerth Billakanti (QUIC) 2022-04-04 17:56 ` Sankeerth Billakanti (QUIC) 2022-03-30 16:02 ` [PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-31 23:23 ` Doug Anderson 2022-03-31 23:23 ` Doug Anderson 2022-04-08 16:14 ` Dmitry Baryshkov 2022-04-08 16:14 ` Dmitry Baryshkov 2022-04-08 17:12 ` Sankeerth Billakanti 2022-04-08 17:12 ` Sankeerth Billakanti 2022-04-08 18:02 ` Dmitry Baryshkov 2022-04-08 18:02 ` Dmitry Baryshkov 2022-03-30 16:02 ` [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-31 23:23 ` Doug Anderson 2022-03-31 23:23 ` Doug Anderson 2022-04-04 13:52 ` Sankeerth Billakanti (QUIC) 2022-04-04 13:52 ` Sankeerth Billakanti (QUIC) 2022-04-04 21:13 ` Dmitry Baryshkov 2022-04-04 21:13 ` Dmitry Baryshkov 2022-04-07 12:40 ` Sankeerth Billakanti (QUIC) 2022-04-07 12:40 ` Sankeerth Billakanti (QUIC) 2022-03-30 16:02 ` [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-31 23:23 ` Doug Anderson 2022-03-31 23:23 ` Doug Anderson 2022-04-04 18:32 ` Sankeerth Billakanti (QUIC) 2022-04-04 18:32 ` Sankeerth Billakanti (QUIC) 2022-04-04 21:15 ` Dmitry Baryshkov 2022-04-04 21:15 ` Dmitry Baryshkov 2022-04-07 12:41 ` Sankeerth Billakanti (QUIC) 2022-04-07 12:41 ` Sankeerth Billakanti (QUIC) 2022-03-30 16:02 ` [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp Sankeerth Billakanti 2022-03-30 16:02 ` Sankeerth Billakanti 2022-03-30 22:08 ` Dmitry Baryshkov 2022-03-30 22:08 ` Dmitry Baryshkov 2022-03-31 6:02 ` Sankeerth Billakanti (QUIC) 2022-03-31 6:02 ` Sankeerth Billakanti (QUIC) 2022-03-31 23:24 ` Doug Anderson 2022-03-31 23:24 ` Doug Anderson 2022-04-04 18:20 ` Sankeerth Billakanti (QUIC) 2022-04-04 18:20 ` Sankeerth Billakanti (QUIC) 2022-04-04 21:29 ` Dmitry Baryshkov 2022-04-04 21:29 ` Dmitry Baryshkov 2022-04-07 14:05 ` Sankeerth Billakanti (QUIC) 2022-04-07 14:05 ` Sankeerth Billakanti (QUIC) 2022-04-08 12:08 ` Dmitry Baryshkov 2022-04-08 12:08 ` Dmitry Baryshkov 2022-04-08 15:50 ` Sankeerth Billakanti 2022-04-08 15:50 ` Sankeerth Billakanti 2022-04-08 16:47 ` Dmitry Baryshkov 2022-04-08 16:47 ` Dmitry Baryshkov 2022-04-08 17:38 ` Sankeerth Billakanti 2022-04-08 17:38 ` Sankeerth Billakanti 2022-04-08 18:06 ` Dmitry Baryshkov 2022-04-08 18:06 ` Dmitry Baryshkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAA8EJppt9XONbgtKfmHmN+==QNqiVJeb8GKJFdZm=yyY-tgmHQ@mail.gmail.com' \ --to=dmitry.baryshkov@linaro.org \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_aravindh@quicinc.com \ --cc=quic_kalyant@quicinc.com \ --cc=quic_khsieh@quicinc.com \ --cc=quic_sbillaka@quicinc.com \ --cc=quic_vproddut@quicinc.com \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ --cc=seanpaul@chromium.org \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.