| Message ID | 1648656179-10347-2-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
On 30/03/2022 19:02, Sankeerth Billakanti wrote: > This patch adds support for generic eDP sink through aux_bus. The eDP/DP > controller driver should support aux transactions originating from the > panel-edp driver and hence should be initialized and ready. > > The panel bridge supporting the panel should be ready before the bridge > connector is initialized. The generic panel probe needs the controller > resources to be enabled to support the aux transactions originating from > the panel probe. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > Changes in v6: > - Remove initialization > - Fix aux_bus node leak > - Split the patches > > drivers/gpu/drm/msm/dp/dp_display.c | 54 +++++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++++--- > drivers/gpu/drm/msm/dp/dp_parser.c | 21 +-------------- > drivers/gpu/drm/msm/dp/dp_parser.h | 1 + > 4 files changed, 60 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 382b3aa..e082d02 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -10,6 +10,7 @@ > #include <linux/component.h> > #include <linux/of_irq.h> > #include <linux/delay.h> > +#include <drm/dp/drm_dp_aux_bus.h> > > #include "msm_drv.h" > #include "msm_kms.h" > @@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > - dp->dp_display.next_bridge = dp->parser->next_bridge; > - > dp->aux->drm_dev = drm; > rc = dp_aux_register(dp->aux); > if (rc) { > @@ -1524,6 +1523,53 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) > } > } > > +static int dp_display_get_next_bridge(struct msm_dp *dp) > +{ > + int rc; > + struct dp_display_private *dp_priv; > + struct device_node *aux_bus; > + struct device *dev; > + > + dp_priv = container_of(dp, struct dp_display_private, dp_display); > + dev = &dp_priv->pdev->dev; > + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > + > + if (aux_bus) { > + dp_display_host_init(dp_priv); > + dp_catalog_ctrl_hpd_config(dp_priv->catalog); > + enable_irq(dp_priv->irq); > + dp_display_host_phy_init(dp_priv); > + > + devm_of_dp_aux_populate_ep_devices(dp_priv->aux); > + > + disable_irq(dp_priv->irq); > + of_node_put(aux_bus); > + } > + > + /* > + * External bridges are mandatory for eDP interfaces: one has to > + * provide at least an eDP panel (which gets wrapped into panel-bridge). > + * > + * For DisplayPort interfaces external bridges are optional, so > + * silently ignore an error if one is not present (-ENODEV). > + */ > + rc = dp_parser_find_next_bridge(dp_priv->parser); > + if (rc == -ENODEV) { > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { The more I think about these conditions, the closer I dislike them (yes, I added this one in one of the patches). I'd suggest to change dp->connector_type to boolean 'is_edp' field use it in all conditions instead. > + DRM_ERROR("eDP: next bridge is not present\n"); > + return rc; > + } > + } else if (rc) { > + if (rc != -EPROBE_DEFER) > + DRM_ERROR("DP: error parsing next bridge: %d\n", rc); > + return rc; > + } > + > + dp->next_bridge = dp_priv->parser->next_bridge; > + > + return 0; > +} > + > int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > struct drm_encoder *encoder) > { > @@ -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; > + > dp_display->bridge = dp_bridge_init(dp_display, dev, encoder); > if (IS_ERR(dp_display->bridge)) { > ret = PTR_ERR(dp_display->bridge); > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index 7ce1aca..5254bd6 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device * > bridge->funcs = &dp_bridge_ops; > bridge->type = dp_display->connector_type; > > - bridge->ops = > - DRM_BRIDGE_OP_DETECT | > - DRM_BRIDGE_OP_HPD | > - DRM_BRIDGE_OP_MODES; > + if (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) { And in this case we can also check dp_display->connector_type (or the suggested dp_display->is_edp) for the uniformity of the code. > + bridge->ops = > + DRM_BRIDGE_OP_DETECT | > + DRM_BRIDGE_OP_HPD | > + DRM_BRIDGE_OP_MODES; I think OP_MODES should be used for eDP, shouldn't it? > + } > > rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); > if (rc) { > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c > index 1056b8d..6317dce 100644 > --- a/drivers/gpu/drm/msm/dp/dp_parser.c > +++ b/drivers/gpu/drm/msm/dp/dp_parser.c > @@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser) > return 0; > } > > -static int dp_parser_find_next_bridge(struct dp_parser *parser) > +int dp_parser_find_next_bridge(struct dp_parser *parser) > { > struct device *dev = &parser->pdev->dev; > struct drm_bridge *bridge; > @@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) > if (rc) > return rc; > > - /* > - * External bridges are mandatory for eDP interfaces: one has to > - * provide at least an eDP panel (which gets wrapped into panel-bridge). > - * > - * For DisplayPort interfaces external bridges are optional, so > - * silently ignore an error if one is not present (-ENODEV). > - */ > - rc = dp_parser_find_next_bridge(parser); > - if (rc == -ENODEV) { > - if (connector_type == DRM_MODE_CONNECTOR_eDP) { > - DRM_ERROR("eDP: next bridge is not present\n"); > - return rc; > - } > - } else if (rc) { > - if (rc != -EPROBE_DEFER) > - DRM_ERROR("DP: error parsing next bridge: %d\n", rc); > - return rc; > - } > - > /* Map the corresponding regulator information according to > * version. Currently, since we only have one supported platform, > * mapping the regulator directly. > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h > index d371bae..091ff41 100644 > --- a/drivers/gpu/drm/msm/dp/dp_parser.h > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h > @@ -140,5 +140,6 @@ struct dp_parser { > * can be parsed using this module. > */ > struct dp_parser *dp_parser_get(struct platform_device *pdev); > +int dp_parser_find_next_bridge(struct dp_parser *parser); > > #endif
Hi, On Wed, Mar 30, 2022 at 4:19 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > + bridge->ops = > > + DRM_BRIDGE_OP_DETECT | > > + DRM_BRIDGE_OP_HPD | > > + DRM_BRIDGE_OP_MODES; > > I think OP_MODES should be used for eDP, shouldn't it? No. It's confusing, but basically to get the power sequencing correct we end up driving the EDID read from the panel driver in the eDP case. -Doug
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. 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. Huh, but I guess you _are_ getting called (indirectly) from dpu_kms_hw_init() and I can't imagine AUX transfers working before that function is called, so maybe I should just accept that it's complicated and let those who understand this driver better confirm that it's OK. ;-) > @@ -140,5 +140,6 @@ struct dp_parser { > * can be parsed using this module. > */ > struct dp_parser *dp_parser_get(struct platform_device *pdev); > +int dp_parser_find_next_bridge(struct dp_parser *parser); Everything else in this file is described w/ kerneldoc. Shouldn't your function also have a kerneldoc comment?
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 > 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. 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. > Huh, but I guess you _are_ getting called (indirectly) from > dpu_kms_hw_init() and I can't imagine AUX transfers working before > that function is called, so maybe I should just accept that it's > complicated and let those who understand this driver better confirm > that it's OK. ;-) > > >> @@ -140,5 +140,6 @@ struct dp_parser { >> * can be parsed using this module. >> */ >> struct dp_parser *dp_parser_get(struct platform_device *pdev); >> +int dp_parser_find_next_bridge(struct dp_parser *parser); > > Everything else in this file is described w/ kerneldoc. Shouldn't your > function also have a kerneldoc comment?
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? I guess conceivably one could re-design the DRM subsystem like that, but I don't think it's trivial. 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. 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. 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? -Doug
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.
Hi, On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > 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. OK, so I misspoke when I said that the modes are populated during probe time for edp-panel. They're not and I guess I managed to confuse myself when I wrote my previous email. Instead they (and everything else that comes from the EDID) isn't needed until the panel's get_modes() is called, as you said. So, ignoring the backlight problem talked about below, we could conceivably delay the reading of the EDID until get_modes. ...but I think something still isn't quite right with your description of how it works. Specifically: 1. I'm 99% certain that get_modes() is called _before_ enable, even for the DP case. I have no idea how that works for you for DP if the clocks / timing signals don't work until enable happens. Aside from my previous observations of this, it also logically makes sense that get_modes() needs to be called before enable(), doesn't it? When enable() is called then don't you already know what mode userspace has picked for you? How can userspace pick a mode to give to enable if you can't query the modes until after enable? 2. As soon as you have told userspace that a display is present then, I believe, it's legal for userspace to ask for the set of available modes. 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. > 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. Actually we _probably_ can do this. Right now it only affects a small subset of panels (those that use AUX backlight) and I can give it a shot if this is the last blocker. ...this is even more awkward than the above, though, because the first first call to get_modes() will actually _cause_ the backlight device to show up. That's not super elegant but it might work OK? > > 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. Even for eDP the AUX transfer shouldn't happen until after HPD is asserted. That's why the AUX transfer function has to wait for HPD. However, for eDP it's a requirement to register/create the AUX bus before HPD is asserted. We went through lots of design discussions and the end result of it all was that we pass the AUX bus to the panel at the panel's probe time. This is something I don't think we want to revisit. Logically there are simply some things that will have to be different for eDP and DP. It really stems from the case that in the lowest power state of eDP that we truly can't tell if the panel is there and thus we have to assume it's there. It also comes from the fact that, in eDP, the panel driver is in charge of doing power sequencing across several regulators / GPIOs including getting the timing right. -Doug
On Mon, 4 Apr 2022 at 23:53, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Sat, Apr 2, 2022 at 1:26 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > 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. > > OK, so I misspoke when I said that the modes are populated during > probe time for edp-panel. They're not and I guess I managed to confuse > myself when I wrote my previous email. Instead they (and everything > else that comes from the EDID) isn't needed until the panel's > get_modes() is called, as you said. So, ignoring the backlight problem > talked about below, we could conceivably delay the reading of the EDID > until get_modes. Yes. > > ...but I think something still isn't quite right with your description > of how it works. Specifically: > > 1. I'm 99% certain that get_modes() is called _before_ enable, even > for the DP case. I have no idea how that works for you for DP if the > clocks / timing signals don't work until enable happens. Aside from my > previous observations of this, it also logically makes sense that > get_modes() needs to be called before enable(), doesn't it? Yes, you are correct here. I also wasn't clear enough about the display clocks/aux clocks being enabled. Of course, it's the display (video) mode clocks not being provided before the enable() call. (well, before the moment between prepare()/pre_enable() and enable()). The side channel buses are a separate story. > When > enable() is called then don't you already know what mode userspace has > picked for you? How can userspace pick a mode to give to enable if you > can't query the modes until after enable? Yes. You are correct here. > > 2. As soon as you have told userspace that a display is present then, > I believe, it's legal for userspace to ask for the set of available > modes. Yes. > > 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. And I also think it might help the ti,sn65dsi86 driver, as it won't have to ensure that gpio is available during the AUX bus probe. BTW, another random idea, before you start coding. We have the bridge's hpd_notify call. Currently it is called only by the means of drm_bridge_connector's HPD mechanism, tied to the bridge registering as DRM_BRIDGE_OP_HPD. It looks to me like it might be a perfect fit for the first aux-bus related reads. We'd need to trigger it manually once and tie it to the new drm_panel_funcs callback, which in turn would probe the aux bus, create backlight, etc. Regarding the Sankeerth's patch. I have been comparing it with the hpd_event_thread()'s calls. It looks to me like we should reuse dp_display_config_hpd() /EV_HPD_INIT_SETUP and maybe others. What I'm trying to say is that if we split AUX probing and first AUX transfers, it would be possible to reuse a significant part of MSM DP HPD machine rather than hacking around it and replicating it manually. > > > > 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. > > Actually we _probably_ can do this. Right now it only affects a small > subset of panels (those that use AUX backlight) and I can give it a > shot if this is the last blocker. > > ...this is even more awkward than the above, though, because the first > first call to get_modes() will actually _cause_ the backlight device > to show up. That's not super elegant but it might work OK? This is really an interesting question. See the hpd_notify suggestion above. > > > > > 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. > > Even for eDP the AUX transfer shouldn't happen until after HPD is > asserted. That's why the AUX transfer function has to wait for HPD. > However, for eDP it's a requirement to register/create the AUX bus > before HPD is asserted. We went through lots of design discussions and > the end result of it all was that we pass the AUX bus to the panel at > the panel's probe time. This is something I don't think we want to > revisit. > > Logically there are simply some things that will have to be different > for eDP and DP. It really stems from the case that in the lowest power > state of eDP that we truly can't tell if the panel is there and thus > we have to assume it's there. It also comes from the fact that, in > eDP, the panel driver is in charge of doing power sequencing across > several regulators / GPIOs including getting the timing right. > > > -Doug -- With best wishes Dmitry
Hi, On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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. > And I also think it might help the ti,sn65dsi86 driver, as it won't > have to ensure that gpio is available during the AUX bus probe. The ti,sn65dsi86 GPIO issue has been solved for a while, though so not sure why we need to do something there? I'm also unclear how it would have helped. In this discussion, we've agreed that the panel driver would still acquire resources during its probe time and the only thing that would be delayed would be the first AUX transfer. The GPIO is a resource here and it's ideal to acquire it at probe time so we could EPROBE_DEFER if needed. > BTW, another random idea, before you start coding. > > We have the bridge's hpd_notify call. Currently it is called only by > the means of drm_bridge_connector's HPD mechanism, tied to the bridge > registering as DRM_BRIDGE_OP_HPD. > It looks to me like it might be a perfect fit for the first aux-bus > related reads. > > We'd need to trigger it manually once and tie it to the new > drm_panel_funcs callback, which in turn would probe the aux bus, > create backlight, etc. > > Regarding the Sankeerth's patch. I have been comparing it with the > hpd_event_thread()'s calls. > It looks to me like we should reuse dp_display_config_hpd() > /EV_HPD_INIT_SETUP and maybe others. > > What I'm trying to say is that if we split AUX probing and first AUX > transfers, it would be possible to reuse a significant part of MSM DP > HPD machine rather than hacking around it and replicating it manually. I'm not sure I completely understand, but I'm pretty wary here. It's my assertion that all of the current "HPD" infrastructure in DRM all relates to the physical presence of the panel. If you start implementing these functions for eDP I think you're going to confuse the heck out of everything. The kernel will think that this is a display that's sometimes not there. Whenever the display is powered off then HPD will be low and it will look like there's no display. Nothing will ever try to power it on because it looks like there's no display. I think your idea is to "trigger once" at bootup and then it all magically works, right? ...but what about after bootup? If you turn the display off for whatever reason (modeset or you simply close the lid of your laptop because you're using an external display) and then you want to use the eDP display again, how do you kickstart the process another time? You can't reboot, and when the display is off the HPD line is low. I can't say it enough times, HPD on eDP _does not mean hot plug detect_. The panel is always there. HPD is really a "panel ready / panel notify" signal for eDP. That's fully what its function is. -Doug
On 05/04/2022 20:02, Doug Anderson wrote: > Hi, > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> 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. A cleaner design might be to split all hotplug event handling from the dp_display, provide a lightweight state machine for the eDP and select which state machine to use depending on the hardware connector type. The dp_display_bind/unbind would probably also be duplicated and receive correct code flows for calling dp_parser_get_next_bridge, etc. Basically that means that depending on the device data we'd use either dp_display_comp_ops or (new) edp_comp_ops. WDYT? >> And I also think it might help the ti,sn65dsi86 driver, as it won't >> have to ensure that gpio is available during the AUX bus probe. > > The ti,sn65dsi86 GPIO issue has been solved for a while, though so not > sure why we need to do something there? I'm also unclear how it would > have helped. In this discussion, we've agreed that the panel driver > would still acquire resources during its probe time and the only thing > that would be delayed would be the first AUX transfer. The GPIO is a > resource here and it's ideal to acquire it at probe time so we could > EPROBE_DEFER if needed. Ack. I agree here. The world at 5 a.m. looks differently :D > > >> BTW, another random idea, before you start coding. >> >> We have the bridge's hpd_notify call. Currently it is called only by >> the means of drm_bridge_connector's HPD mechanism, tied to the bridge >> registering as DRM_BRIDGE_OP_HPD. >> It looks to me like it might be a perfect fit for the first aux-bus >> related reads. >> >> We'd need to trigger it manually once and tie it to the new >> drm_panel_funcs callback, which in turn would probe the aux bus, >> create backlight, etc. >> >> Regarding the Sankeerth's patch. I have been comparing it with the >> hpd_event_thread()'s calls. >> It looks to me like we should reuse dp_display_config_hpd() >> /EV_HPD_INIT_SETUP and maybe others. >> >> What I'm trying to say is that if we split AUX probing and first AUX >> transfers, it would be possible to reuse a significant part of MSM DP >> HPD machine rather than hacking around it and replicating it manually. > > I'm not sure I completely understand, but I'm pretty wary here. It's > my assertion that all of the current "HPD" infrastructure in DRM all > relates to the physical presence of the panel. If you start > implementing these functions for eDP I think you're going to confuse > the heck out of everything. The kernel will think that this is a > display that's sometimes not there. Whenever the display is powered > off then HPD will be low and it will look like there's no display. > Nothing will ever try to power it on because it looks like there's no > display. > > I think your idea is to "trigger once" at bootup and then it all > magically works, right? ...but what about after bootup? If you turn > the display off for whatever reason (modeset or you simply close the > lid of your laptop because you're using an external display) and then > you want to use the eDP display again, how do you kickstart the > process another time? You can't reboot, and when the display is off > the HPD line is low. > > I can't say it enough times, HPD on eDP _does not mean hot plug > detect_. The panel is always there. HPD is really a "panel ready / > panel notify" signal for eDP. That's fully what its function is. Too many things being called HPD. I meant to trigger drm's hpd handling, which is not tied to the HPD pin for panel-edp. HPD pin is checked during panel runtime resume. However... let's probably go a simpler way.
Hi, On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On 05/04/2022 20:02, Doug Anderson wrote: > > Hi, > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> 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. This is why I think we need to move to Runtime PM to manage this. Basically: 1. When an AUX transfer happens, you grab a PM runtime reference that _that_ powers up the PHY. 2. At the end of the AUX transfer function, you do a "put_autosuspend". Then it becomes not a hack, right? > A cleaner design might be to split all hotplug event handling from the > dp_display, provide a lightweight state machine for the eDP and select > which state machine to use depending on the hardware connector type. The > dp_display_bind/unbind would probably also be duplicated and receive > correct code flows for calling dp_parser_get_next_bridge, etc. > Basically that means that depending on the device data we'd use either > dp_display_comp_ops or (new) edp_comp_ops. > > WDYT? I don't think I know the structure of the MSM DP code to make a definitive answer here. I think I'd have to see a patch. However I'd agree in general terms that we need some different flows for the two. ;-) We definitely want to limit the differences but some of them will be unavoidable... -Doug
Hi Dmitry and Doug, > Hi, > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On 05/04/2022 20:02, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> 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. > > This is why I think we need to move to Runtime PM to manage this. Basically: > > 1. When an AUX transfer happens, you grab a PM runtime reference that > _that_ powers up the PHY. > > 2. At the end of the AUX transfer function, you do a "put_autosuspend". > > Then it becomes not a hack, right? > > pm runtime ops needs to be implemented for both eDP and DP. This change take good amount of planning and code changes as it affects DP also. Because this patch series consist of basic eDP changes for SC7280 bootup, shall we take this pm_runtime implementation in subsequent patch series? > > A cleaner design might be to split all hotplug event handling from the > > dp_display, provide a lightweight state machine for the eDP and select > > which state machine to use depending on the hardware connector type. > > The dp_display_bind/unbind would probably also be duplicated and > > receive correct code flows for calling dp_parser_get_next_bridge, etc. > > Basically that means that depending on the device data we'd use either > > dp_display_comp_ops or (new) edp_comp_ops. > > > > WDYT? > > I don't think I know the structure of the MSM DP code to make a definitive > answer here. I think I'd have to see a patch. However I'd agree in general > terms that we need some different flows for the two. > ;-) We definitely want to limit the differences but some of them will be > unavoidable... > > > -Doug Thank you, Sankeerth
Hi, On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > Hi Dmitry and Doug, > > > Hi, > > > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On 05/04/2022 20:02, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > > > <dmitry.baryshkov@linaro.org> 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. > > > > This is why I think we need to move to Runtime PM to manage this. Basically: > > > > 1. When an AUX transfer happens, you grab a PM runtime reference that > > _that_ powers up the PHY. > > > > 2. At the end of the AUX transfer function, you do a "put_autosuspend". > > > > Then it becomes not a hack, right? > > > > > > pm runtime ops needs to be implemented for both eDP and DP. This change > take good amount of planning and code changes as it affects DP also. > > Because this patch series consist of basic eDP changes for SC7280 bootup, > shall we take this pm_runtime implementation in subsequent patch series? Dmitry is the real decision maker here, but in my opinion it would be OK to get something landed first that worked OK and wasn't taking us too far in the wrong direction and then we could get a follow up patch to move to pm_runtime.
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) > <quic_sbillaka@quicinc.com> wrote: >> >> Hi Dmitry and Doug, >> >>> Hi, >>> >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov >>> <dmitry.baryshkov@linaro.org> wrote: >>>> >>>> On 05/04/2022 20:02, Doug Anderson wrote: >>>>> Hi, >>>>> >>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov >>>>> <dmitry.baryshkov@linaro.org> 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. So this is boiling down to why or how panel-eDP was originally designed. >>> This is why I think we need to move to Runtime PM to manage this. Basically: >>> >>> 1. When an AUX transfer happens, you grab a PM runtime reference that >>> _that_ powers up the PHY. This will not be trivial and needs to be scoped out as sankeerth said but if the above is the only concern, why do we need to do this? There seems to be an explanation why we are doing this and its not a hack. How would Dmitry's rework address this? We need some RFC to conclude on that first. >>> >>> 2. At the end of the AUX transfer function, you do a "put_autosuspend". >>> >>> Then it becomes not a hack, right? >>> >>> >> >> pm runtime ops needs to be implemented for both eDP and DP. This change >> take good amount of planning and code changes as it affects DP also. >> >> Because this patch series consist of basic eDP changes for SC7280 bootup, >> shall we take this pm_runtime implementation in subsequent patch series? > > Dmitry is the real decision maker here, but in my opinion it would be > OK to get something landed first that worked OK and wasn't taking us > too far in the wrong direction and then we could get a follow up patch > to move to pm_runtime. I would say the discussion changed into a direction of implementing pm-runtime because the current patch series does what it takes to adhere to panel-eDP's design along with aux bus requirements of PHY needing to be on. So doug, to answer your questions here: "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." Yes PHY needs to be absolutely on and configured before aux transfers. If we want to change that up to stop reading the panel_id in the panel probe() and do it later, perhaps some of the changes done here are not needed. It only seems reasonable that we first prototype that in a separate patch even a RFC perhaps and take this further as these set of changes are needed for basic display functionality on sc7280 chromebooks. Let us know what are the concerns with doing it in a follow up change. Thanks Abhinav
Hi, On Thu, Apr 7, 2022 at 1:11 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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) > > <quic_sbillaka@quicinc.com> wrote: > >> > >> Hi Dmitry and Doug, > >> > >>> Hi, > >>> > >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> > >>>> On 05/04/2022 20:02, Doug Anderson wrote: > >>>>> Hi, > >>>>> > >>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > >>>>> <dmitry.baryshkov@linaro.org> 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. 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. > So this is boiling down to why or how panel-eDP was originally designed. > > >>> This is why I think we need to move to Runtime PM to manage this. Basically: > >>> > >>> 1. When an AUX transfer happens, you grab a PM runtime reference that > >>> _that_ powers up the PHY. > > This will not be trivial and needs to be scoped out as sankeerth said > but if the above is the only concern, why do we need to do this? There > seems to be an explanation why we are doing this and its not a hack. > > How would Dmitry's rework address this? We need some RFC to conclude on > that first. > > >>> > >>> 2. At the end of the AUX transfer function, you do a "put_autosuspend". > >>> > >>> Then it becomes not a hack, right? > >>> > >>> > >> > >> pm runtime ops needs to be implemented for both eDP and DP. This change > >> take good amount of planning and code changes as it affects DP also. > >> > >> Because this patch series consist of basic eDP changes for SC7280 bootup, > >> shall we take this pm_runtime implementation in subsequent patch series? > > > > Dmitry is the real decision maker here, but in my opinion it would be > > OK to get something landed first that worked OK and wasn't taking us > > too far in the wrong direction and then we could get a follow up patch > > to move to pm_runtime. > > I would say the discussion changed into a direction of implementing > pm-runtime because the current patch series does what it takes to adhere > to panel-eDP's design along with aux bus requirements of PHY needing to > be on. > > So doug, to answer your questions here: > > "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." > > Yes PHY needs to be absolutely on and configured before aux transfers. > > If we want to change that up to stop reading the panel_id in the panel > probe() and do it later, perhaps some of the changes done here are not > needed. > > It only seems reasonable that we first prototype that in a separate > patch even a RFC perhaps and take this further as these set of changes > are needed for basic display functionality on sc7280 chromebooks. > > Let us know what are the concerns with doing it in a follow up change. As per above, I'm not objecting to it being a follow-up change, but I do believe it's the right design and will lead to an overall cleaner solution. I think I even mentioned in my reviews that the current patch series seems to "scattershot" enable resources and that's how we end up with patches like patch #5 in this series ("drm/msm/dp: prevent multiple votes for dp resources"). IMO there should be be a 1-to-1 mapping between "turn on resources" and "turn off resources" and it should be reference counted. So if your codepath turned on resources then it's up to your codepath to turn resources off when done. If a seconde code path might be running at the same time then it should also turn on/off resources itself. ...and it should all be managed by pm_runtime which is _exactly designed_ for this specific use case. -Doug
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 <quic_abhinavk@quicinc.com> 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) >>> <quic_sbillaka@quicinc.com> wrote: >>>> >>>> Hi Dmitry and Doug, >>>> >>>>> Hi, >>>>> >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov >>>>> <dmitry.baryshkov@linaro.org> wrote: >>>>>> >>>>>> On 05/04/2022 20:02, Doug Anderson wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov >>>>>>> <dmitry.baryshkov@linaro.org> 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. > 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? > >> So this is boiling down to why or how panel-eDP was originally designed. >> >>>>> This is why I think we need to move to Runtime PM to manage this. Basically: >>>>> >>>>> 1. When an AUX transfer happens, you grab a PM runtime reference that >>>>> _that_ powers up the PHY. >> >> This will not be trivial and needs to be scoped out as sankeerth said >> but if the above is the only concern, why do we need to do this? There >> seems to be an explanation why we are doing this and its not a hack. >> >> How would Dmitry's rework address this? We need some RFC to conclude on >> that first. >> >>>>> >>>>> 2. At the end of the AUX transfer function, you do a "put_autosuspend". >>>>> >>>>> Then it becomes not a hack, right? >>>>> >>>>> >>>> >>>> pm runtime ops needs to be implemented for both eDP and DP. This change >>>> take good amount of planning and code changes as it affects DP also. >>>> >>>> Because this patch series consist of basic eDP changes for SC7280 bootup, >>>> shall we take this pm_runtime implementation in subsequent patch series? >>> >>> Dmitry is the real decision maker here, but in my opinion it would be >>> OK to get something landed first that worked OK and wasn't taking us >>> too far in the wrong direction and then we could get a follow up patch >>> to move to pm_runtime. >> >> I would say the discussion changed into a direction of implementing >> pm-runtime because the current patch series does what it takes to adhere >> to panel-eDP's design along with aux bus requirements of PHY needing to >> be on. >> >> So doug, to answer your questions here: >> >> "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." >> >> Yes PHY needs to be absolutely on and configured before aux transfers. >> >> If we want to change that up to stop reading the panel_id in the panel >> probe() and do it later, perhaps some of the changes done here are not >> needed. >> >> It only seems reasonable that we first prototype that in a separate >> patch even a RFC perhaps and take this further as these set of changes >> are needed for basic display functionality on sc7280 chromebooks. >> >> Let us know what are the concerns with doing it in a follow up change. > > As per above, I'm not objecting to it being a follow-up change, but I > do believe it's the right design and will lead to an overall cleaner > solution. I think I even mentioned in my reviews that the current > patch series seems to "scattershot" enable resources and that's how we > end up with patches like patch #5 in this series ("drm/msm/dp: prevent > multiple votes for dp resources"). IMO there should be be a 1-to-1 > mapping between "turn on resources" and "turn off resources" and it > should be reference counted. So if your codepath turned on resources > then it's up to your codepath to turn resources off when done. If a > seconde code path might be running at the same time then it should > also turn on/off resources itself. ...and it should all be managed by > pm_runtime which is _exactly designed_ for this specific use case. > Agreed on this topic, moving to pm_runtime() will club all resources in one place and make things cleaner that way. Complexity of it obviously needs to be evaluated to check the effort Vs rewards. But it will still not address the original concern of this thread that powering up the PHY before the probe() is a hack. "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." We would still end up doing that under the hood of pm_runtime. And thats why its an improvement and not a necessity qualifying it for a separate change. > -Doug
Hi, On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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 <quic_abhinavk@quicinc.com> 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) > >>> <quic_sbillaka@quicinc.com> wrote: > >>>> > >>>> Hi Dmitry and Doug, > >>>> > >>>>> Hi, > >>>>> > >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > >>>>> <dmitry.baryshkov@linaro.org> wrote: > >>>>>> > >>>>>> On 05/04/2022 20:02, Doug Anderson wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > >>>>>>> <dmitry.baryshkov@linaro.org> 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. 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. 3. Any other init (setting up pixel clocks) can continue to happen where it is today. > > 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. The above is probably not a giant deal, but I think long term it would be better to be acquiring resources earlier.
On Thu, 7 Apr 2022 at 23:11, Abhinav Kumar <quic_abhinavk@quicinc.com> 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) > > <quic_sbillaka@quicinc.com> wrote: > >> > >> Hi Dmitry and Doug, > >> > >>> Hi, > >>> > >>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > >>> <dmitry.baryshkov@linaro.org> wrote: > >>>> > >>>> On 05/04/2022 20:02, Doug Anderson wrote: > >>>>> Hi, > >>>>> > >>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > >>>>> <dmitry.baryshkov@linaro.org> 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. > > So this is boiling down to why or how panel-eDP was originally designed. Well, it's not just panel-edp. It boils down to the DP-AUX bus design vs DRM design. However in Doug's defense I should admit that I can not come up with a significantly cleaner solution. Just to emphasise (or to repeat): for all other display panels/bridges, we either do not use a sidechannel bus or the sidechannel bus (i2c, spi, platform) is managed outside of the DRM framework. Thus it's possible to create the source in the drm's driver probe path and then in the component's bind() path check (and return an error) if the sink device wasn't yet probed successfully. The source can then either communicate with the sink using the sidechannel bus provided elsewhere or (e.g. in case of purely DSI panel), defer communication till the moment the display path is fully available (after encoder enablement). For the DP/eDP the sidechannel (DP AUX) bus is closer to the display path. It can not be used separately. For DP we can defer all communication till the moment we know (through the DP/USB-C/etc hotplug mechanism) that the sink is really available and running. The eDP is being caught in between these two approaches: For example sn65dsi86 and tegra have separate dpaux and separate bridge/output devices. Thus dpaux'es probe() methos populates DP AUX bus, then a separate device checks whether the panel/bridge has become available in its own probe() method. The ps8640 driver looks 'working by coincidence'. It calls dp_aux_populate, then immediately after the function returns it checks for the panel. If panel-edp is built as a module, the probe might fail easily. The anx7625 driver has the same kind of issue. The DP AUX bus is populated from the probe() and after some additional work the panel is being checked. This design is fragile and from my quick glance it can break (or be broken) too easy. It reminds me of our drm msm 'probe' loops preventing the device to boot completely if the dsi bridge/panel could not be probed in time. If we talk about DP AUX bus EP drivers, both panel-edp and panel-samsung-atna33xc20 (the only EP drivers present in Linux master) expect that they can access DPCD from the probe method. Now back to Qualcomm DP driver. We do not have a separate dpaux entity. If leave aside the idea of adding a separate 'bus available' callback, I see two possible solutions: - Implement separate lightweight eDP driver sharing source with the DP driver. Make it skip all the DP HPD craziness, init PHY and call devm_of_dp_aux_ep_populate_ep_devices() from the probe method, check in the bind method that the next bridge is available. However this can potentially break ports which can be used either in the DP or in eDP mode. or - Modify existing Qualcomm DP driver to return -EPROBE_DEFER from dp_aux_transfer() if the AUX bus is not available. Make the driver init PHY from probe() if it is running in the eDP mode. Populate DP AUX bus from probe(). Check for the next bridge in dp_bind(). There might be potentially other possibilities, but I think you have my main idea. Create the bus in probe(), check for the bridge in bind(). or - Create a bus at some point in bind. Forbid (and document that) AUX access from EP probe(). Access it only from get_modes(). > > >>> This is why I think we need to move to Runtime PM to manage this. Basically: > >>> > >>> 1. When an AUX transfer happens, you grab a PM runtime reference that > >>> _that_ powers up the PHY. > > This will not be trivial and needs to be scoped out as sankeerth said > but if the above is the only concern, why do we need to do this? There > seems to be an explanation why we are doing this and its not a hack. > > How would Dmitry's rework address this? We need some RFC to conclude on > that first. Just to put things clear: I do not have plans to work on either of my suggestions at least in the next few months. I do not have eDP hardware at hand. > > >>> > >>> 2. At the end of the AUX transfer function, you do a "put_autosuspend". > >>> > >>> Then it becomes not a hack, right? > >>> > >>> > >> > >> pm runtime ops needs to be implemented for both eDP and DP. This change > >> take good amount of planning and code changes as it affects DP also. > >> > >> Because this patch series consist of basic eDP changes for SC7280 bootup, > >> shall we take this pm_runtime implementation in subsequent patch series? > > > > Dmitry is the real decision maker here, but in my opinion it would be > > OK to get something landed first that worked OK and wasn't taking us > > too far in the wrong direction and then we could get a follow up patch > > to move to pm_runtime. > > I would say the discussion changed into a direction of implementing > pm-runtime because the current patch series does what it takes to adhere > to panel-eDP's design along with aux bus requirements of PHY needing to > be on. > > So doug, to answer your questions here: > > "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." > > Yes PHY needs to be absolutely on and configured before aux transfers. > > If we want to change that up to stop reading the panel_id in the panel > probe() and do it later, perhaps some of the changes done here are not > needed. > > It only seems reasonable that we first prototype that in a separate > patch even a RFC perhaps and take this further as these set of changes > are needed for basic display functionality on sc7280 chromebooks. > > Let us know what are the concerns with doing it in a follow up change. > > Thanks > > Abhinav -- With best wishes Dmitry
On Fri, 8 Apr 2022 at 02:35, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Apr 7, 2022 at 3:03 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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 <quic_abhinavk@quicinc.com> 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) > > >>> <quic_sbillaka@quicinc.com> wrote: > > >>>> > > >>>> Hi Dmitry and Doug, > > >>>> > > >>>>> Hi, > > >>>>> > > >>>>> On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > > >>>>> <dmitry.baryshkov@linaro.org> wrote: > > >>>>>> > > >>>>>> On 05/04/2022 20:02, Doug Anderson wrote: > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > >>>>>>> <dmitry.baryshkov@linaro.org> 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.
Hi, On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > The ps8640 driver looks 'working by coincidence'. It calls > dp_aux_populate, then immediately after the function returns it checks > for the panel. If panel-edp is built as a module, the probe might fail > easily. > The anx7625 driver has the same kind of issue. The DP AUX bus is > populated from the probe() and after some additional work the panel is > being checked. > This design is fragile and from my quick glance it can break (or be > broken) too easy. It reminds me of our drm msm 'probe' loops > preventing the device to boot completely if the dsi bridge/panel could > not be probed in time. I did spend some time thinking about this, at least for ps8640. I believe that as long as the panel's probe isn't asynchronous. Basically if the panel isn't ready then ps8640 should return and we'll retry later. I do remember the probe loops that we used to have with msm and I don't _think_ this would trigger it. That being said, if we need to separate out the AUX bus into a sub-device like we did in sn65dsi86 we certainly could. -Doug
Hi, On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > 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) I guess my thought was that in DP you could still create the AUX bus at probe time. Then for DP you just return an instant "transfer failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed elsewhere) when we try to do an AUX transfer then we delay until HPD is there. So we can still acquire resources (clocks, PHY, io maps, etc) at probe time for DP and create the AUX bus, right? It will just return "-ENODEV" if HPD isn't asserted and you're DP? -Doug
On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > The ps8640 driver looks 'working by coincidence'. It calls > > dp_aux_populate, then immediately after the function returns it checks > > for the panel. If panel-edp is built as a module, the probe might fail > > easily. > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > populated from the probe() and after some additional work the panel is > > being checked. > > This design is fragile and from my quick glance it can break (or be > > broken) too easy. It reminds me of our drm msm 'probe' loops > > preventing the device to boot completely if the dsi bridge/panel could > > not be probed in time. > > I did spend some time thinking about this, at least for ps8640. I > believe that as long as the panel's probe isn't asynchronous. By panel probe (as a probe of any device) is potentially asynchronous. As in your example, the PWM might not be present, the regulator probe might have been delayed, the panel-edp might be built as a module, which is not present for some reason. > Basically if the panel isn't ready then ps8640 should return and we'll > retry later. I do remember the probe loops that we used to have with > msm and I don't _think_ this would trigger it. I don't have proof here. But as I wrote, this thing might break at some point. > That being said, if we need to separate out the AUX bus into a > sub-device like we did in sn65dsi86 we certainly could. Ideally we should separate the "bridge" subdevice, like it was done in sn65dsi86. So that the aux host probes, registers the EP device, then the bridge device can not be probed (and thus the drm_bridge is not created) until the next bridge becomes available.
On Fri, 8 Apr 2022 at 03:26, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Apr 7, 2022 at 4:46 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > > 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) > > I guess my thought was that in DP you could still create the AUX bus > at probe time. Then for DP you just return an instant "transfer > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed > elsewhere) when we try to do an AUX transfer then we delay until HPD > is there. I think panel-edp would already handle the delay, so we do not need to have this logic in the DP driver. > So we can still acquire resources (clocks, PHY, io maps, etc) at probe > time for DP and create the AUX bus, right? It will just return > "-ENODEV" if HPD isn't asserted and you're DP? Yes, please. I still suppose that we'd need a separate case to power_on eDP's PHY during the probe time. Maybe I'm mistaken here.
Hi, On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > I guess my thought was that in DP you could still create the AUX bus > > at probe time. Then for DP you just return an instant "transfer > > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed > > elsewhere) when we try to do an AUX transfer then we delay until HPD > > is there. > > I think panel-edp would already handle the delay, so we do not need to > have this logic in the DP driver. There's a whole discussion about this between Stephen and me in patch #5 ("drm/msm/dp: wait for hpd high before any sink interaction"). Basically: * If panel HPD is hooked up to the dedicated HPD pin on the eDP controller then the panel driver doesn't have a way to read it. * We can't leverage the existing "HPD" query functions in DRM because those indicate whether a panel is _physically_ connected. For eDP, it always is. For now the rule is that the AUX transfer function is in charge of waiting for HPD for eDP if the dedicated HPD pin is used. If we want to re-invent this we could, but that system works, isn't _too_ ugly, and we're already making big enough changes in this series. > > So we can still acquire resources (clocks, PHY, io maps, etc) at probe > > time for DP and create the AUX bus, right? It will just return > > "-ENODEV" if HPD isn't asserted and you're DP? > > Yes, please. I still suppose that we'd need a separate case to > power_on eDP's PHY during the probe time. Maybe I'm mistaken here. I think the ideal way is to do it like Kieran's proposal for sn65dsi86: https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/ * When enabling HPD (physical hot plug detect) in the hpd_enable() callback you do a pm_runtime_get(). You do the pm_runtime_put_autosuspend() when disabling. This is only used for DP since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP. * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX transfer routine. While holding the pm_runtime reference we check HPD. For DP we return immediately if HPD isn't asserted. For eDP, we delay. * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in post_disable. For DP this will add a 2nd refcount (since we probably were holding the reference for HPD). For eDP this will cause us to power on. * If there's any other time we need to read HW registers, and we aren't guaranteed to already have a pm_runtime reference (like during probe), we can do a temporary pm_runtime_get() / pm_runtime_put_autosuspend().
Hi, On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > The ps8640 driver looks 'working by coincidence'. It calls > > > dp_aux_populate, then immediately after the function returns it checks > > > for the panel. If panel-edp is built as a module, the probe might fail > > > easily. > > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > > populated from the probe() and after some additional work the panel is > > > being checked. > > > This design is fragile and from my quick glance it can break (or be > > > broken) too easy. It reminds me of our drm msm 'probe' loops > > > preventing the device to boot completely if the dsi bridge/panel could > > > not be probed in time. > > > > I did spend some time thinking about this, at least for ps8640. I > > believe that as long as the panel's probe isn't asynchronous. > > By panel probe (as a probe of any device) is potentially asynchronous. > As in your example, the PWM might not be present, the regulator probe > might have been delayed, the panel-edp might be built as a module, > which is not present for some reason. Well, in those cases it's not exactly asynchronous, or at least not in the way I was thinking about. Either it will work now, or we should try again later when more drivers have probed. The case I was worried about was: 1. We call devm_of_dp_aux_populate_ep_devices() 2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel in the background 3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting for the panel's probe to finish. I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS. I was less worried about the resources missing since I thought that would be handled by the normal probe deferral case. IIRC the "infinite loop" that we used to end up with MSM's probe was because something about the way the MSM DRM driver worked would cause the deferral mechanisms to retry instantly each time we deferred. I don't remember exactly what triggered it, but I don't _think_ that's true for ps8640? > > Basically if the panel isn't ready then ps8640 should return and we'll > > retry later. I do remember the probe loops that we used to have with > > msm and I don't _think_ this would trigger it. > > I don't have proof here. But as I wrote, this thing might break at some point. > > > That being said, if we need to separate out the AUX bus into a > > sub-device like we did in sn65dsi86 we certainly could. > > Ideally we should separate the "bridge" subdevice, like it was done in > sn65dsi86. > So that the aux host probes, registers the EP device, then the bridge > device can not be probed (and thus the drm_bridge is not created) > until the next bridge becomes available. You're definitely right, that's best. I was hesitant to force the issue during review of the ps8640 because it adds a bunch of complexity and didn't _seem_ to be needed. Maybe it makes sense to just code it up, though... -Doug
On Fri, 8 Apr 2022 at 16:56, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Fri, 8 Apr 2022 at 03:28, Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > The ps8640 driver looks 'working by coincidence'. It calls > > > > dp_aux_populate, then immediately after the function returns it checks > > > > for the panel. If panel-edp is built as a module, the probe might fail > > > > easily. > > > > The anx7625 driver has the same kind of issue. The DP AUX bus is > > > > populated from the probe() and after some additional work the panel is > > > > being checked. > > > > This design is fragile and from my quick glance it can break (or be > > > > broken) too easy. It reminds me of our drm msm 'probe' loops > > > > preventing the device to boot completely if the dsi bridge/panel could > > > > not be probed in time. > > > > > > I did spend some time thinking about this, at least for ps8640. I > > > believe that as long as the panel's probe isn't asynchronous. > > > > By panel probe (as a probe of any device) is potentially asynchronous. > > As in your example, the PWM might not be present, the regulator probe > > might have been delayed, the panel-edp might be built as a module, > > which is not present for some reason. > > Well, in those cases it's not exactly asynchronous, or at least not in > the way I was thinking about. Either it will work now, or we should > try again later when more drivers have probed. The case I was worried > about was: > > 1. We call devm_of_dp_aux_populate_ep_devices() > > 2. devm_of_dp_aux_populate_ep_devices() kicks off a probe to the panel > in the background > > 3. devm_of_dp_aux_populate_ep_devices() returns to us without waiting > for the panel's probe to finish. > > I think that's possible if the panel driver sets PROBE_PREFER_ASYNCHRONOUS. That would be a separate story, yes. However the general case is still valid: one can not guarantee that the panel device is available immediately after aux bus population. So ps8640 works at this moment and in the particular configuration. > > I was less worried about the resources missing since I thought that > would be handled by the normal probe deferral case. IIRC the "infinite > loop" that we used to end up with MSM's probe was because something > about the way the MSM DRM driver worked would cause the deferral > mechanisms to retry instantly each time we deferred. I don't remember > exactly what triggered it, but I don't _think_ that's true for ps8640? I'm not sure either. If you have a system with that bridge, it can be easily tried by returning -EPROBE_DEFER from the panel driver's probe(). For the msm driver it was the following sequence of events: - mdss probes - mdss creates subdevices including dsi host - subdevices are probed - mdss drivers tries to perform component binding - dsi host determines that the next item is not available - it returns -EPROBE_DEFER to the component bind - mdss reverts registration of subdevices, returning probe defer. However as there were devices added to the device list, the deferral list was retried immediately. Thus we faced the probe loop. I think this looks close to the ps8640, but I wouldn't bet on that. > > > Basically if the panel isn't ready then ps8640 should return and we'll > > > retry later. I do remember the probe loops that we used to have with > > > msm and I don't _think_ this would trigger it. > > > > I don't have proof here. But as I wrote, this thing might break at some point. > > > > > That being said, if we need to separate out the AUX bus into a > > > sub-device like we did in sn65dsi86 we certainly could. > > > > Ideally we should separate the "bridge" subdevice, like it was done in > > sn65dsi86. > > So that the aux host probes, registers the EP device, then the bridge > > device can not be probed (and thus the drm_bridge is not created) > > until the next bridge becomes available. > > You're definitely right, that's best. I was hesitant to force the > issue during review of the ps8640 because it adds a bunch of > complexity and didn't _seem_ to be needed. Maybe it makes sense to > just code it up, though... As I wrote, the test is easy. If the system boots fine, there is no need to fix that immediately. However I think in general we should stop depending on the panel being available right after populating the aux bus. -- With best wishes Dmitry
On Fri, 8 Apr 2022 at 16:43, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > > I guess my thought was that in DP you could still create the AUX bus > > > at probe time. Then for DP you just return an instant "transfer > > > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed > > > elsewhere) when we try to do an AUX transfer then we delay until HPD > > > is there. > > > > I think panel-edp would already handle the delay, so we do not need to > > have this logic in the DP driver. > > There's a whole discussion about this between Stephen and me in patch > #5 ("drm/msm/dp: wait for hpd high before any sink interaction"). > Basically: > > * If panel HPD is hooked up to the dedicated HPD pin on the eDP > controller then the panel driver doesn't have a way to read it. I refreshed that dialog. I must admit, I have missed the fact that the HPD pin might not be visible as the GPIO pin. > * We can't leverage the existing "HPD" query functions in DRM because > those indicate whether a panel is _physically_ connected. For eDP, it > always is. Yes, I was thinking about (mis)using the drm_bridge_connector_hpd_notify() for generic HPD-related notifications (to tell eDP that it should check the current state). I have abandoned that idea. > For now the rule is that the AUX transfer function is in charge of > waiting for HPD for eDP if the dedicated HPD pin is used. If we want > to re-invent this we could, but that system works, isn't _too_ ugly, > and we're already making big enough changes in this series. The is_hpd_asserted() looks like a good callback for the aux bus. It will allow the panel driver to check if the panel is powered up (in the absence of the GPIO pin). > > > So we can still acquire resources (clocks, PHY, io maps, etc) at probe > > > time for DP and create the AUX bus, right? It will just return > > > "-ENODEV" if HPD isn't asserted and you're DP? > > > > Yes, please. I still suppose that we'd need a separate case to > > power_on eDP's PHY during the probe time. Maybe I'm mistaken here. > > I think the ideal way is to do it like Kieran's proposal for sn65dsi86: > > https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/ > > * When enabling HPD (physical hot plug detect) in the hpd_enable() > callback you do a pm_runtime_get(). You do the > pm_runtime_put_autosuspend() when disabling. This is only used for DP > since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP. > > * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX > transfer routine. While holding the pm_runtime reference we check HPD. > For DP we return immediately if HPD isn't asserted. For eDP, we delay. > > * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in > post_disable. For DP this will add a 2nd refcount (since we probably > were holding the reference for HPD). For eDP this will cause us to > power on. > > * If there's any other time we need to read HW registers, and we > aren't guaranteed to already have a pm_runtime reference (like during > probe), we can do a temporary pm_runtime_get() / > pm_runtime_put_autosuspend(). This looks good. I'd be more than welcome to review such series. Note: I think this would require using drm_bridge_connector_enable_hpd() in the DP code. Hopefully at some point we would be able to move all drm_bridge_connector calls to the core msm layer. -- With best wishes Dmitry
Hi Doug and Dmitry On 4/8/2022 7:58 AM, Dmitry Baryshkov wrote: > On Fri, 8 Apr 2022 at 16:43, Doug Anderson <dianders@chromium.org> wrote: >> >> Hi, >> >> On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov >> <dmitry.baryshkov@linaro.org> wrote: >>> >>>> I guess my thought was that in DP you could still create the AUX bus >>>> at probe time. Then for DP you just return an instant "transfer >>>> failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed >>>> elsewhere) when we try to do an AUX transfer then we delay until HPD >>>> is there. >>> >>> I think panel-edp would already handle the delay, so we do not need to >>> have this logic in the DP driver. >> >> There's a whole discussion about this between Stephen and me in patch >> #5 ("drm/msm/dp: wait for hpd high before any sink interaction"). >> Basically: >> >> * If panel HPD is hooked up to the dedicated HPD pin on the eDP >> controller then the panel driver doesn't have a way to read it. > > I refreshed that dialog. I must admit, I have missed the fact that the > HPD pin might not be visible as the GPIO pin. > >> * We can't leverage the existing "HPD" query functions in DRM because >> those indicate whether a panel is _physically_ connected. For eDP, it >> always is. > > Yes, I was thinking about (mis)using the > drm_bridge_connector_hpd_notify() for generic HPD-related > notifications (to tell eDP that it should check the current state). I > have abandoned that idea. > >> For now the rule is that the AUX transfer function is in charge of >> waiting for HPD for eDP if the dedicated HPD pin is used. If we want >> to re-invent this we could, but that system works, isn't _too_ ugly, >> and we're already making big enough changes in this series. > > The is_hpd_asserted() looks like a good callback for the aux bus. > It will allow the panel driver to check if the panel is powered up (in > the absence of the GPIO pin). > >>>> So we can still acquire resources (clocks, PHY, io maps, etc) at probe >>>> time for DP and create the AUX bus, right? It will just return >>>> "-ENODEV" if HPD isn't asserted and you're DP? >>> >>> Yes, please. I still suppose that we'd need a separate case to >>> power_on eDP's PHY during the probe time. Maybe I'm mistaken here. >> >> I think the ideal way is to do it like Kieran's proposal for sn65dsi86: >> >> https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/ >> >> * When enabling HPD (physical hot plug detect) in the hpd_enable() >> callback you do a pm_runtime_get(). You do the >> pm_runtime_put_autosuspend() when disabling. This is only used for DP >> since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP. >> >> * We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX >> transfer routine. While holding the pm_runtime reference we check HPD. >> For DP we return immediately if HPD isn't asserted. For eDP, we delay. >> >> * We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in >> post_disable. For DP this will add a 2nd refcount (since we probably >> were holding the reference for HPD). For eDP this will cause us to >> power on. >> >> * If there's any other time we need to read HW registers, and we >> aren't guaranteed to already have a pm_runtime reference (like during >> probe), we can do a temporary pm_runtime_get() / >> pm_runtime_put_autosuspend(). > > This looks good. I'd be more than welcome to review such series. > > Note: I think this would require using > drm_bridge_connector_enable_hpd() in the DP code. > Hopefully at some point we would be able to move all > drm_bridge_connector calls to the core msm layer. > -- > With best wishes > Dmitry Thanks for the proposals. In general I would break up this task as follows: 1) Re-factoring dp/edp parsing code to move it to probe ( its currently done in bind ). So not sure what dependencies we will uncover there. Nonetheless, lets assume for now it can be done. 2) Then bind all the power resources needed for AUX in pm_runtime_ops 3) Handle EPROBE_DEFER cases of the panel-eDP aux device 4) Probably the biggest from our point of view --- makes sure none of this breaks DP/eDP Since QC will be taking ownership of all of this, I would still suggest land this series first so that basic display functionality on sc7280 chromebooks works, unblocks more developers and this program and we can internally evaluate all of this and post the changes as-and-when ready for review. So, I suggest/request acking this current one after fixing the other comments (unrelated to this re-factor) which have been given so far ofcourse, as we all agree this is not breaking and seems pretty reasonable short term. Doug, you can track this re-factor with a different bug so that all this discussion remains intact. Thanks Abhinav
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 382b3aa..e082d02 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -10,6 +10,7 @@ #include <linux/component.h> #include <linux/of_irq.h> #include <linux/delay.h> +#include <drm/dp/drm_dp_aux_bus.h> #include "msm_drv.h" #include "msm_kms.h" @@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct device *master, goto end; } - dp->dp_display.next_bridge = dp->parser->next_bridge; - dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux); if (rc) { @@ -1524,6 +1523,53 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) } } +static int dp_display_get_next_bridge(struct msm_dp *dp) +{ + int rc; + struct dp_display_private *dp_priv; + struct device_node *aux_bus; + struct device *dev; + + dp_priv = container_of(dp, struct dp_display_private, dp_display); + dev = &dp_priv->pdev->dev; + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); + + if (aux_bus) { + dp_display_host_init(dp_priv); + dp_catalog_ctrl_hpd_config(dp_priv->catalog); + enable_irq(dp_priv->irq); + dp_display_host_phy_init(dp_priv); + + devm_of_dp_aux_populate_ep_devices(dp_priv->aux); + + disable_irq(dp_priv->irq); + of_node_put(aux_bus); + } + + /* + * External bridges are mandatory for eDP interfaces: one has to + * provide at least an eDP panel (which gets wrapped into panel-bridge). + * + * For DisplayPort interfaces external bridges are optional, so + * silently ignore an error if one is not present (-ENODEV). + */ + rc = dp_parser_find_next_bridge(dp_priv->parser); + if (rc == -ENODEV) { + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { + DRM_ERROR("eDP: next bridge is not present\n"); + return rc; + } + } else if (rc) { + if (rc != -EPROBE_DEFER) + DRM_ERROR("DP: error parsing next bridge: %d\n", rc); + return rc; + } + + dp->next_bridge = dp_priv->parser->next_bridge; + + return 0; +} + int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder) { @@ -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; + dp_display->bridge = dp_bridge_init(dp_display, dev, encoder); if (IS_ERR(dp_display->bridge)) { ret = PTR_ERR(dp_display->bridge); diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 7ce1aca..5254bd6 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device * bridge->funcs = &dp_bridge_ops; bridge->type = dp_display->connector_type; - bridge->ops = - DRM_BRIDGE_OP_DETECT | - DRM_BRIDGE_OP_HPD | - DRM_BRIDGE_OP_MODES; + if (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) { + bridge->ops = + DRM_BRIDGE_OP_DETECT | + DRM_BRIDGE_OP_HPD | + DRM_BRIDGE_OP_MODES; + } rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc) { diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 1056b8d..6317dce 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; } -static int dp_parser_find_next_bridge(struct dp_parser *parser) +int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev; struct drm_bridge *bridge; @@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc; - /* - * External bridges are mandatory for eDP interfaces: one has to - * provide at least an eDP panel (which gets wrapped into panel-bridge). - * - * For DisplayPort interfaces external bridges are optional, so - * silently ignore an error if one is not present (-ENODEV). - */ - rc = dp_parser_find_next_bridge(parser); - if (rc == -ENODEV) { - if (connector_type == DRM_MODE_CONNECTOR_eDP) { - DRM_ERROR("eDP: next bridge is not present\n"); - return rc; - } - } else if (rc) { - if (rc != -EPROBE_DEFER) - DRM_ERROR("DP: error parsing next bridge: %d\n", rc); - return rc; - } - /* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform, * mapping the regulator directly. diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index d371bae..091ff41 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -140,5 +140,6 @@ struct dp_parser { * can be parsed using this module. */ struct dp_parser *dp_parser_get(struct platform_device *pdev); +int dp_parser_find_next_bridge(struct dp_parser *parser); #endif
This patch adds support for generic eDP sink through aux_bus. The eDP/DP controller driver should support aux transactions originating from the panel-edp driver and hence should be initialized and ready. The panel bridge supporting the panel should be ready before the bridge connector is initialized. The generic panel probe needs the controller resources to be enabled to support the aux transactions originating from the panel probe. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- Changes in v6: - Remove initialization - Fix aux_bus node leak - Split the patches drivers/gpu/drm/msm/dp/dp_display.c | 54 +++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++++--- drivers/gpu/drm/msm/dp/dp_parser.c | 21 +-------------- drivers/gpu/drm/msm/dp/dp_parser.h | 1 + 4 files changed, 60 insertions(+), 26 deletions(-)