This series adds support for generic eDP panel over aux_bus.
These changes are dependent on the following series in order: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=620127&s... https://patchwork.kernel.org/project/linux-arm-msm/list/?series=616587&s... https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&s...
Sankeerth Billakanti (8): drm/msm/dp: Add eDP support via aux_bus drm/msm/dp: wait for hpd high before aux transaction drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP drm/msm/dp: avoid handling masked interrupts drm/msm/dp: prevent multiple votes for dp resources drm/msm/dp: remove unnecessary delay during boot drm/msm/dp: Support edp/dp without hpd drm/msm/dp: Handle eDP mode_valid differently from dp
drivers/gpu/drm/msm/dp/dp_aux.c | 13 ++++- drivers/gpu/drm/msm/dp/dp_aux.h | 3 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 35 ++++++++++--- drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 99 ++++++++++++++++++++++++++++++++++--- 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 + 8 files changed, 143 insertions(+), 40 deletions(-)
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) { + 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
On 30/03/2022 19:02, Sankeerth Billakanti wrote:
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.
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.
I think OP_MODES should be used for eDP, shouldn't it?
Hi,
On Wed, Mar 30, 2022 at 4:19 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
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:
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. ;-)
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:
It's called modeset_init() as it initializes KMS objects used by DP driver. We have similar functions for dsi and hdmi
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.
Hi,
On Sat, Apr 2, 2022 at 3:37 AM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
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?
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.
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:
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.
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.
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.
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:
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.
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?
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:
Yes.
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.
Yes. You are correct here.
Yes.
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.
This is really an interesting question. See the hpd_notify suggestion above.
-- With best wishes Dmitry
Hi,
On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
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.
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:
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?
Ack. I agree here. The world at 5 a.m. looks differently :D
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:
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?
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,
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?
Thank you, Sankeerth
Hi,
On Thu, Apr 7, 2022 at 7:19 AM Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
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:
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 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.
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:
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.
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:
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.
Correct and by delaying the panel-edp's probe(), we are doing exactly that?
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:
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.
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 Fri, 8 Apr 2022 at 02:35, Doug Anderson dianders@chromium.org wrote:
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.
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.
Yes.
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:46 PM 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.
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:26, Doug Anderson dianders@chromium.org wrote:
I think panel-edp would already handle the delay, so we do not need to have this logic in the DP driver.
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:
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.
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@id...
* 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().
On Fri, 8 Apr 2022 at 16:43, Doug Anderson dianders@chromium.org wrote:
I refreshed that dialog. I must admit, I have missed the fact that the HPD pin might not be visible as the GPIO pin.
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.
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).
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:
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
On Thu, 7 Apr 2022 at 23:11, Abhinav Kumar quic_abhinavk@quicinc.com wrote:
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().
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.
-- With best wishes Dmitry
Hi,
On Thu, Apr 7, 2022 at 4:36 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
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
On Fri, 8 Apr 2022 at 03:28, Doug Anderson dianders@chromium.org wrote:
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.
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.
Hi,
On Fri, Apr 8, 2022 at 5:13 AM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
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?
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:
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'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.
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
The source device should ensure the sink is ready before proceeding to read the sink capability or performing any aux transactions. The sink will indicate its readiness by asserting the HPD line. The controller driver needs to wait for the hpd line to be asserted by the sink before performing any aux transactions.
The eDP sink is assumed to be always connected. It needs power from the source and its HPD line will be asserted only after the panel is powered on. The panel power will be enabled from the panel-edp driver and only after that, the hpd line will be asserted.
Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd line gets asserted to indicate the sink is connected and ready. Hence there is no need to wait for the hpd line to be asserted for a DP sink.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com ---
Changes in v6: - Wait for hpd high only for eDP - Split into smaller patches
drivers/gpu/drm/msm/dp/dp_aux.c | 13 ++++++++++++- drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++- drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +++++++++++++ drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 3 ++- 5 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..a217c80 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -36,6 +36,7 @@ struct dp_aux_private { bool initted; u32 offset; u32 segment; + bool is_edp;
struct drm_dp_aux dp_aux; }; @@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux, goto exit; }
+ if (aux->is_edp) { + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); + if (ret) { + DRM_DEBUG_DP("Panel not ready for aux transactions\n"); + goto exit; + } + } + dp_aux_update_offset_and_segment(aux, msg); dp_aux_transfer_helper(aux, msg, true);
@@ -491,7 +500,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) drm_dp_aux_unregister(dp_aux); }
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog) +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog, + bool is_edp) { struct dp_aux_private *aux;
@@ -506,6 +516,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
init_completion(&aux->comp); aux->cmd_busy = false; + aux->is_edp = is_edp; mutex_init(&aux->mutex);
aux->dev = dev; diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h index 82afc8d..c99aeec 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.h +++ b/drivers/gpu/drm/msm/dp/dp_aux.h @@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux); void dp_aux_deinit(struct drm_dp_aux *dp_aux); void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog); +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog, + bool is_edp); void dp_aux_put(struct drm_dp_aux *aux);
#endif /*__DP_AUX_H_*/ diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index fac815f..b6add4e 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog) phy_calibrate(phy); }
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) +{ + u32 state; + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + /* poll for hpd connected status every 2ms and timeout after 500ms */ + return readl_poll_timeout(catalog->io->dp_controller.aux.base + + REG_DP_DP_HPD_INT_STATUS, + state, state & DP_DP_HPD_STATE_STATUS_CONNECTED, + 2000, 500000); +} + static void dump_regs(void __iomem *base, int len) { int i; diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 7dea101..45140a3 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.h +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h @@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog *dp_catalog); void dp_catalog_aux_reset(struct dp_catalog *dp_catalog); void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable); void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog); +int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog); u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
/* DP Controller APIs */ diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index e082d02..274bbcf 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -755,6 +755,7 @@ static void dp_display_deinit_sub_modules(struct dp_display_private *dp) static int dp_init_sub_modules(struct dp_display_private *dp) { int rc = 0; + bool is_edp = (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP); struct device *dev = &dp->pdev->dev; struct dp_usbpd_cb *cb = &dp->usbpd_cb; struct dp_panel_in panel_in = { @@ -798,7 +799,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp) goto error; }
- dp->aux = dp_aux_get(dev, dp->catalog); + dp->aux = dp_aux_get(dev, dp->catalog, is_edp); if (IS_ERR(dp->aux)) { rc = PTR_ERR(dp->aux); DRM_ERROR("failed to initialize aux, rc = %d\n", rc);
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
The source device should ensure the sink is ready before proceeding to read the sink capability or performing any aux transactions. The sink
s/performing/perform
Adding a comment about _why_ you're doing this just for eDP would probably be a good idea. Like maybe:
/* * For eDP it's important to give a reasonably long wait here for HPD * to be asserted. This is because the panel driver may have _just_ * turned on the panel and then tried to do an AUX transfer. The panel * driver has no way of knowing when the panel is ready, so it's up * to us to wait. For DP we never get into this situation so let's * avoid ever doing the extra long wait for DP. */
nit: I think your indentation of the 2nd line isn't quite right.
nit: I think your indentation of the 2nd line isn't quite right.
Things are pretty much nits, so FWIW:
Reviewed-by: Douglas Anderson dianders@chromium.org
Hi Doug,
Okay. Will add it
I moved bool is_edp into the next line. In vim , it was sowing fine. I'll check
I'll check
Things are pretty much nits, so FWIW:
Reviewed-by: Douglas Anderson dianders@chromium.org
Thank you, Sankeerth
The panel-edp enables the eDP panel power during probe, get_modes and enable. The eDP connect and disconnect interrupts for the eDP/DP controller are directly dependent on panel power. As eDP display can be assumed as always connected, the controller driver can skip the eDP connect and disconnect interrupts. Any disruption in the link status will be indicated via the IRQ_HPD interrupts.
So, the eDP controller driver can just enable the IRQ_HPD and replug interrupts. The DP controller driver still needs to enable all the interrupts.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_catalog.c | 4 ---- drivers/gpu/drm/msm/dp/dp_display.c | 24 ++++++++++++++++++++++-- 2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index b6add4e..3c16f95 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -582,10 +582,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
- /* enable HPD plug and unplug interrupts */ - dp_catalog_hpd_config_intr(dp_catalog, - DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true); - /* Configure REFTIMER and enable it */ reftimer |= DP_DP_HPD_REFTIMER_ENABLE; dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 274bbcf..888ff03 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -677,7 +677,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */ - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n", dp->dp_display.connector_type, state); @@ -1087,10 +1088,17 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
static void dp_display_config_hpd(struct dp_display_private *dp) { - dp_display_host_init(dp); + dp_catalog_ctrl_hpd_config(dp->catalog);
+ /* Enable plug and unplug interrupts only for external DisplayPort */ + if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + dp_catalog_hpd_config_intr(dp->catalog, + DP_DP_HPD_PLUG_INT_MASK | + DP_DP_HPD_UNPLUG_INT_MASK, + true); + /* Enable interrupt first time * we are leaving dp clocks on during disconnect * and never disable interrupt @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) dp_catalog_ctrl_hpd_config(dp->catalog);
+ if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort) + dp_catalog_hpd_config_intr(dp->catalog, + DP_DP_HPD_PLUG_INT_MASK | + DP_DP_HPD_UNPLUG_INT_MASK, + true); + if (dp_catalog_link_is_connected(dp->catalog)) { /* * set sink to normal operation mode -- D0 @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) return; }
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) + dp_hpd_plug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */ @@ -1703,6 +1720,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display = container_of(dp, struct dp_display_private, dp_display);
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) + dp_hpd_unplug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
Hi,
On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
nit: why are there two blank lines above?
Should you add a "pre_enable" and do it there? That would make it more symmetric with the fact that you have the countertpart in "post_disable".
Overall: I'm probably not familiar enough with this code to give it a full review. I'm hoping that Dmitry knows it well enough... ;-)
-Doug
Hi Doug,
Will remove a blank line.
We want to handle the screen off or eDP disable like a cable unplug so that it can be integrated into the dp code. So, we can move plug_handle into pre_enable and the unplug_handle to pre_disable.
If we do unplug in post_disable, we need to handle the state changes also. It will complicate the driver code.
Thank you, Sankeerth
The interrupt register will still reflect the connect and disconnect interrupt status without generating an actual HW interrupt. The controller driver should not handle those masked interrupts.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 3c16f95..1809ce2 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog); - int isr = 0; + int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS); dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK, (isr & DP_DP_HPD_INT_MASK)); + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr; + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask); }
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
I suspect that the logic is inverted here. Shouldn't it be:
return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
?
Hi Dmitry,
The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE0000000 and the value of the read interrupt mask variable could be is 0xF.
The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0. The HPD status is indicated via a different read-only register REG_DP_DP_HPD_INT_STATUS, bits 31:29.
isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
Thank you, Sankeerth
Hi Dmitry,
I believe the confusion occurred because the DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange the definitions below.
#define REG_DP_DP_HPD_INT_MASK (0x0000000C) #define DP_DP_HPD_PLUG_INT_MASK (0x00000001) #define DP_DP_IRQ_HPD_INT_MASK (0x00000002) #define DP_DP_HPD_REPLUG_INT_MASK (0x00000004) #define DP_DP_HPD_UNPLUG_INT_MASK (0x00000008) #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \ DP_DP_IRQ_HPD_INT_MASK | \ DP_DP_HPD_REPLUG_INT_MASK | \ DP_DP_HPD_UNPLUG_INT_MASK)
Below are status bits from register REG_DP_DP_HPD_INT_STATUS
#define DP_DP_HPD_STATE_STATUS_CONNECTED (0x40000000) #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000)
DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
For DP, we want to enable all interrupts. So the programmed mask value is 0xF. We want to return 0x40000001 for connect and 8 for disconnect
For eDP, we want to disable the connect and disconnect interrupts. So, the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x40000000 (or 0x20000000 based on hpd line status) and 0 for eDP connect and disconnect respectively.
Thank you, Sankeerth
On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti sbillaka@qti.qualcomm.com wrote:
Ugh, excuse me please. I meant:
return isr & (mask | ~DP_DP_HPD_INT_MASK);
The aux_bus support with the dp_display driver will enable the dp resources during msm_dp_modeset_init. The host_init has to return early if the core is already initialized to prevent putting an additional vote for the dp controller resources.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_display.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 888ff03..798b30b 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -420,6 +420,11 @@ static void dp_display_host_init(struct dp_display_private *dp) dp->dp_display.connector_type, dp->core_initialized, dp->phy_initialized);
+ if (dp->core_initialized) { + DRM_DEBUG_DP("DP core already initialized\n"); + return; + } + dp_power_init(dp->power, false); dp_ctrl_reset_irq_ctrl(dp->ctrl, true); dp_aux_init(dp->aux); @@ -432,6 +437,11 @@ static void dp_display_host_deinit(struct dp_display_private *dp) dp->dp_display.connector_type, dp->core_initialized, dp->phy_initialized);
+ if (!dp->core_initialized) { + DRM_DEBUG_DP("DP core not initialized\n"); + return; + } + dp_ctrl_reset_irq_ctrl(dp->ctrl, false); dp_aux_deinit(dp->aux); dp_power_deinit(dp->power);
Hi,
On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
I'm not a huge fan of this but I'll leave it up to Dmitry. In general it feels like there should be _a_ place that enables these resources. Checks like this make it feel like we just scattershot enabling resources in a bunch of random places instead of coming up with the design for enabling them in the right place.
In any case, if we do end up landing this patch, it sure feels like it needs to move earlier in the patch series, right? This patch shouldn't hurt even without the other patches in the series but if you apply the earlier patches in the series without this one then you'll have a bug, right? That means this needs to come earlier.
-Doug
On 01/04/2022 02:23, Doug Anderson wrote:
I'd prefer to see a check for eDP in dp_display_config_hpd(). Or even better to see that this function isn't called for eDP at all.
This needs to be called when eDP is not using the aux_bus path. If the eDP panel is given as a separate panel driver, then the resources need to be enabled here.
If we don't want to support eDP without aux_bus, then we can skip this function.
Thank you, Sankeerth
On Fri, 8 Apr 2022 at 20:12, Sankeerth Billakanti sbillaka@qti.qualcomm.com wrote:
I think it's up to you to decide, if it's necessary or not. But if it is, please change it accordingly.
Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP event.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 798b30b..8bafdd0 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1526,7 +1526,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp_hpd_event_setup(dp);
- dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); }
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
Hi,
On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP event.
Tell me more and put it in the commit message! Why did it used to be necessary and why is it no longer necessary? Inquiring minds want to know.
-Doug
Hi Doug,
Okay. I will add proper description. The DP phy is shared with usb and executing the dp phy_init before the usb phy_init was causing usb devices to not function.
Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event was executed. So, we had scheduled it to execute after 10 second to let the usb driver complete the phy_init first.
Kuogee made the below change to move the DP phy_init to execute after the DP is connected https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648-8448-2-g...
So, there is no need for the DP driver to wait 10 seconds for the phy initialization anymore.
eDP PHY is not shared with usb. So, it can be programmed anytime, hence not needing any delay.
-Doug
Thank you, Sankeerth
On Mon, 4 Apr 2022 at 16:53, Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
I always wondered, how does this work for the 4-lane DP dongles, where there is no USB mode/lanes?
Hi Dmitry,
For 4 lane DP, the DP driver overrides the phy programming. For 2 lanes DP, the usb phy programming is leveraged.
I believe the usb controller needs to enable both usb3 (qmp phy) and usb2 (hs phy) before DP phy_init. If one of the usb phy init fails, the usb controller will not enumerate and will cause usb2 devices (like keyboard or mouse) to not work. The usb controller probe was failing because DP was turning on (voting) a clock which was supposed to be initialized by the usb driver.
We did not see any issue with DP because the programming anyway gets overwritten but due to usb controller failure, usb devices did not work.
Some eDP sinks or platform boards will not support hpd. This patch adds support for those cases.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 1809ce2..8f1fc71 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog)
int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog) { - u32 state; + u32 state, hpd_en; struct dp_catalog_private *catalog = container_of(dp_catalog, struct dp_catalog_private, dp_catalog);
+ hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL); + hpd_en &= DP_DP_HPD_CTRL_HPD_EN; + + /* no-hpd case */ + if (!hpd_en) + return 0; + /* poll for hpd connected status every 2ms and timeout after 500ms */ return readl_poll_timeout(catalog->io->dp_controller.aux.base + REG_DP_DP_HPD_INT_STATUS, @@ -586,8 +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) reftimer |= DP_DP_HPD_REFTIMER_ENABLE; dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
- /* Enable HPD */ - dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); + /* Enable HPD if supported*/ + if (!of_property_read_bool(catalog->dev->of_node, "no-hpd")) + dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, + DP_DP_HPD_CTRL_HPD_EN); }
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
Hi,
On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
Some eDP sinks or platform boards will not support hpd. This patch adds support for those cases.
You could say more, like:
If we're not using HPD then _both_ the panel node and the eDP controller node will have "no-hpd". This tells the eDP panel code to hardcode the maximum possible delay for a panel to power up and tells the eDP driver that it should continue to do transfers even if HPD isn't asserted.
I don't think this is a particularly lightweight operation. It's literally iterating through all of our device tree properties and doing string compares on them. ...but this function is called somewhat often, isn't it? It feels like the kind of thing that should happen at probe time and be stored in a boolean.
...and then you can use that same boolean in dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the register value, right?
-Doug
Hi Doug,
Okay. I will add it
It is called twice for DP. Once while booting through a thread scheduled from kms_obj_init and when resuming from PM suspend.
With aux_bus addition, this function will be called thrice for eDP. Once during bootup with aux_bus, then from scheduled event from kms_obj_init and pm resume like DP.
I will check if we can use a no-hpd Boolean flag instead.
-Doug
Thank you, Sankeerth
On Mon, 4 Apr 2022 at 21:32, Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
As the driver has a separate dp_parser code, it might be a good fit to parse the DT once and then to use boolean flag afterwards.
The panel-edp driver modes needs to be validated differently from DP because the link capabilities are not available for EDP by that time.
Signed-off-by: Sankeerth Billakanti quic_sbillaka@quicinc.com --- drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 8bafdd0..f9c7d9a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, return -EINVAL; }
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) + return MODE_CLOCK_HIGH; + return MODE_OK; + } + if ((dp->max_pclk_khz <= 0) || (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || (mode->clock > dp->max_pclk_khz))
On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti quic_sbillaka@quicinc.com wrote:
This should not be necessary after https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1. Could you please check?
Hi Dmitry,
The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need to return early for eDP because unlike DP, eDP context will not have the information about the number of lanes and link clock.
So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if is_eDP is set.
Hi,
On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
I haven't walked through all the relevant code but something you said above sounds strange. You say that for eDP we don't have info about the number of lanes? We _should_.
It's certainly possible to have a panel that supports _either_ 1 or 2 lanes but then only physically connect 1 lane to it. ...or you could have a panel that supports 2 or 4 lanes and you only connect 1 lane. See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes but if a "data-lanes" property is present then we can use that to know that fewer lanes are physically connected.
It's also possible to connect more lanes to a panel than it supports. You could connect 2 lanes to it but then it only supports 1. This case needs to be handled as well...
-Doug
Hi Doug,
I was referring to the checks we do for DP in dp_bridge_mode_valid. We check if the Link bandwidth can support the pixel bandwidth. For an external DP connection, the Initial DPCD/EDID read after cable connection will return the sink capabilities like link rate, lane count and bpp information that are used to we filter out the unsupported modes from the list of modes from EDID.
For eDP case, the dp driver performs the first dpcd read during bridge_enable. The dp_bridge_mode_valid function is executed before bridge_enable and hence does not have the full link or the sink capabilities information like external DP connection, by then.
So, we need to proceed with the reported mode for eDP.
-Doug
On Mon, 4 Apr 2022 at 21:21, Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
It sounds to me like we should emulate the HPD event for eDP to be handled earlier than the get_modes()/prepare() calls are attempted. However this might open another can of worms.
So, we need to proceed with the reported mode for eDP.
Well... Even if during the first call to get_modes() the DPCD is not read, during subsequent calls the driver has necessary information, so it can proceed with all the checks, can't it?
Hi Dmitry,
For DP, the HPD handler mainly initiates link training and gets the EDID.
Before adding support for a separate eDP panel, we had discussed about this internally and decided to emulate eDP HPD during enable(). Main reason being, eDP power is guaranteed to be on only after bridge_enable(). So, eDP link training can happen and sustain only after bridge_enable().
Emulating HPD before/during get_modes will not have any effect because:
1. get_modes() will go to panel's get_modes() function to power on read EDID
2. panel power will be turned off after get_modes(). Panel power off will reset every write transaction in DPCD. anyway invalidating link training
3. mode_valid will land in dp driver but panel will not be powered on at that time and we cannot do aux transfers or DPCD read writes.
get_modes() currently does not land in DP driver. It gets executed in panel bridge. But the mode_valid() comes to DP driver to check the controller compatibility.
Thank you, Sankeerth
On Thu, 7 Apr 2022 at 17:05, Sankeerth Billakanti (QUIC) quic_sbillaka@quicinc.com wrote:
As we have seen, the term HPD is significantly overloaded. What do you want to emulate?
I tend to agree with Doug here. eDP link power status should be handled by the pm_runtime_autosuspend with grace period being high enough to cover the timeslot between get_mode() and enable().
panel-edp already does most of required work.
Why do we need to perform AUX writes in mode_valid?
Yes, this is correct. the DP's mode_valid() knows the hardware limitations (max clock speed, amount of lanes, etc) and thus it can decide whether the mode is supported by the whole chain or not. We should not skip such checks for the eDP.
Hi Dmitry,
On DP, we use HPD event for link training and EDID read.
I understood that you wanted me to emulate HPD event before get_modes() but because the panel power is controlled by panel-edp, whatever programming we do on the sink side will be reset when panel power will be turned off by the pm_runtime_put_autosuspend() of the panel-edp in panel_edp_get_modes().
The eDP controller resources are enabled through the host_init() and the link resources need to be powered on for doing link training, which needs to happen in the enable() with generic panel-edp.
I am trying to justify why we cannot have mode_valid() implementation similar to DP for eDP. The detect() and get_modes() are called from panel bridge and panel-edp.c respectively. The first eDP specific call which will land in the dp_driver is mode_valid(), in which the controller cannot perform aux access because the panel will not be powered-on.
As the panel-power and backlight are panel resources, we are not enabling/voting for them from the DP/eDP controller driver.
For eDP, we have no information about the number of lanes or the link rate supported We only know the max lanes from the data-lanes DT property.
On Fri, 8 Apr 2022 at 18:50, Sankeerth Billakanti sbillaka@qti.qualcomm.com wrote:
The pm_runtime_put_autosuspend() wouldn't suspend the device immediately. It will be suspended after the grace period finished, if nobody resumes the devices again. This is how it works in the sn65dsi86 driver. It sets the timeout delay long enough, so that get_modes and pre_enable would typically work together without suspending the host.
nothing wrong with that up to now
I fail to understand why you'd like to perform aux access from mode_valid at all.
As the panel-power and backlight are panel resources, we are not enabling/voting for them from the DP/eDP controller driver.
correct
If the device connects just a single line to the eDP panel, the DT will be changed to list that single lane. It looks like we have to call dp_panel_read_sink_caps() somewhere for the eDP case. For the DP case the HPD callbacks do this work.
No, mode_valid doesn't look like a proper place. We already have read modes, so the AUX bus has been powered for some time. We could do it earlier.
Okay. We are not implementing a bridge pre_enable currently
I don't want to perform it in mode_valid. I am just saying that, apart from mode_valid(), there is no other eDP call (other than aux_transfer) which will land in the DP driver before the mode_set(). So, currently there is no function in which we can get the eDP sink capabilities before enable(). So, we assume the mode will be supported if the pixel clock is less than the max clock of 675MHz.
Correct, we have to do it earlier. But is there some function in which we can get the dp_panel_read_sink_caps() before get_modes?
A way could be to implement the mode_valid also in panel-eDP along with the get_modes. We can read the sink capabilities in get_modes in panel-edp.c and check in the mode_valid() in panel-edp.c.
I also feel the mode_valid() needs to check if a controller can support it rather than the panel. So, I cannot find a way where to get the sink caps now before the mode_set() or enable()
On Fri, 8 Apr 2022 at 20:38, Sankeerth Billakanti sbillaka@qti.qualcomm.com wrote:
Anywhere after you have the reference to the next_bridge, you can be sure that the panel is present. So you can power up the AUX bus, read the caps, and (auto-)suspend it again.
dri-devel@lists.freedesktop.org
-
Abhinav Kumar -
Dmitry Baryshkov -
Doug Anderson -
Sankeerth Billakanti -
Sankeerth Billakanti -
Sankeerth Billakanti (QUIC)