This patch addresses pre-existing issues that came up during the
review process of Sankeerth's series trying to add eDP for Qualcomm
SoCs [1].
It's really sorta two series but jammed into one. The first two
patches fix a problem with ps8640 when the panel doesn't finish
probing right away. The rest of the patches attempt to improve how eDP
panel drivers deal with the HPD signal. NOTE: if everyone hates the
"generic driver" that I added in the first patch, I have a different
version that just adds uses the Linux auxiliary bus stright in
ps8640. I'm happy to switch back to that, but it seemed like a buncha
copy-pasta that I was hoping to avoid.
I haven't done a crazy amount of testing with this, but it seems to
work and I wanted to get something out there. I'll try to do some more
testing next week. This is why I added the tag "RFC". It's entirely
possibled that I've actually caught all the bugs and this is great,
but I just wanted to be sure.
This _doesn't_ attempt to fix the Analogix driver. If this works out,
ideally someone can post a patch up to do that.
[1] https://lore.kernel.org/r/[email protected]/
Douglas Anderson (6):
drm/dp: Helpers to make it easier for drivers to use DP AUX bus
properly
drm/bridge: parade-ps8640: Break probe in two to handle DP AUX better
drm/dp: Add is_hpd_asserted() callback to struct drm_dp_aux
drm/panel-edp: Take advantage of is_hpd_asserted() in struct
drm_dp_aux
drm/panel: atna33xc20: Take advantage of is_hpd_asserted() in struct
drm_dp_aux
drm/bridge: parade-ps8640: Provide is_hpd_asserted() in struct
drm_dp_aux
drivers/gpu/drm/bridge/parade-ps8640.c | 87 +++++----
drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++++++++++++++++-
drivers/gpu/drm/panel/panel-edp.c | 37 +++-
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 35 +++-
include/drm/dp/drm_dp_aux_bus.h | 58 ++++++
include/drm/dp/drm_dp_helper.h | 14 ++
6 files changed, 353 insertions(+), 43 deletions(-)
--
2.35.1.1178.g4f1659d476-goog
This implements the callback added by the patch ("drm/dp: Add
is_hpd_asserted() callback to struct drm_dp_aux").
With this change and all the two "DP AUX Endpoint" drivers changed to
use is_hpd_asserted(), we no longer need to have an long delay in the
AUX transfer function. It's up to the panel code to make sure that the
panel is powered now. If someone tried to call the aux transfer
function without making sure the panel is powered we'll just get a
normal transfer failure.
We'll still keep the "ensure" for HPD in the pre_enable()
function. Though it's probably not actually needed there, this driver
is used in the old mode (pre-DP AUX Endpoints) and it may be important
for those cases. If nothing else, it shouldn't cause any big problems.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 96e883985608..13c3cb5d34f3 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -190,6 +190,22 @@ static int ps8640_ensure_hpd(struct ps8640 *ps_bridge)
return ret;
}
+static bool ps8640_is_hpd_asserted(struct drm_dp_aux *aux)
+{
+ struct ps8640 *ps_bridge = aux_to_ps8640(aux);
+ struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
+ struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;
+ unsigned int status = 0;
+
+ pm_runtime_get_sync(dev);
+ regmap_read(map, PAGE2_GPIO_H, &status);
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ /* GPIO9 signals HPD. See ps8640_ensure_hpd() */
+ return status & PS_GPIO9;
+}
+
static ssize_t ps8640_aux_transfer_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -324,9 +340,7 @@ static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
int ret;
pm_runtime_get_sync(dev);
- ret = ps8640_ensure_hpd(ps_bridge);
- if (!ret)
- ret = ps8640_aux_transfer_msg(aux, msg);
+ ret = ps8640_aux_transfer_msg(aux, msg);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
@@ -679,6 +693,7 @@ static int ps8640_probe(struct i2c_client *client)
ps_bridge->aux.name = "parade-ps8640-aux";
ps_bridge->aux.dev = dev;
ps_bridge->aux.transfer = ps8640_aux_transfer;
+ ps_bridge->aux.is_hpd_asserted = ps8640_is_hpd_asserted;
drm_dp_aux_init(&ps_bridge->aux);
pm_runtime_enable(dev);
--
2.35.1.1178.g4f1659d476-goog
As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
patch and also in the past in commit a1e3667a9835 ("drm/bridge:
ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
DP AUX bus properly we really need two "struct device"s. One "struct
device" is in charge of providing the DP AUX bus and the other is
where we'll try to get a reference to the newly probed endpoint
devices.
In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
is already broken up into several "struct devices" anyway because it
also provides a PWM and some GPIOs. Adding one more wasn't that
difficult / ugly.
When I tried to do the same solution in parade-ps8640, it felt like I
was copying too much boilerplate code. I made the realization that I
didn't _really_ need a separate "driver" for each person that wanted
to do the same thing. By putting all the "driver" related code in a
common place then we could save a bit of hassle. This change
effectively adds a new "ep_client" driver that can be used by
anyone. The devices instantiated by this driver will just call through
to the probe/remove/shutdown calls provided.
At the moment, the "ep_client" driver is backed by the Linux auxiliary
bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
want to expose this to clients, though, so as far as clients are
concerned they get a vanilla "struct device".
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++++++++++++++++++++++++++-
include/drm/dp/drm_dp_aux_bus.h | 58 ++++++++++
2 files changed, 222 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
index 415afce3cf96..5386ceacf133 100644
--- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
+++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
@@ -12,6 +12,7 @@
* to perform transactions on that bus.
*/
+#include <linux/auxiliary_bus.h>
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *drv)
}
EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
+/* -----------------------------------------------------------------------------
+ * DP AUX EP Client
+ */
+
+struct dp_aux_ep_client_data {
+ struct dp_aux_ep_client *client;
+ struct auxiliary_device adev;
+};
+
+static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
+ const struct auxiliary_device_id *id)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (!data->client->probe)
+ return 0;
+
+ return data->client->probe(&adev->dev, data->client);
+}
+
+static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (data->client->remove)
+ data->client->remove(&adev->dev, data->client);
+}
+
+static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
+{
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ if (data->client->shutdown)
+ data->client->shutdown(&adev->dev, data->client);
+}
+
+static void dp_aux_ep_client_dev_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+ struct dp_aux_ep_client_data *data =
+ container_of(adev, struct dp_aux_ep_client_data, adev);
+
+ kfree(data);
+}
+
+/**
+ * dp_aux_register_ep_client() - Register an DP AUX EP client
+ * @client: The structure describing the client. It's the client's
+ * responsibility to keep this memory around until
+ * dp_aux_unregister_ep_client() is called, either explicitly or
+ * implicitly via devm.
+ *
+ * See the description of "struct dp_aux_ep_client" for a full explanation
+ * of when you should use this and why.
+ *
+ * Return: 0 if no error or negative error code.
+ */
+int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
+{
+ struct dp_aux_ep_client_data *data;
+ int ret;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->client = client;
+ data->adev.name = "ep_client";
+ data->adev.dev.parent = client->aux->dev;
+ data->adev.dev.release = dp_aux_ep_client_dev_release;
+ device_set_of_node_from_dev(&data->adev.dev, client->aux->dev);
+
+ ret = auxiliary_device_init(&data->adev);
+ if (ret) {
+ /*
+ * NOTE: if init doesn't fail then it takes ownership
+ * of memory and this kfree() is magically part of
+ * auxiliary_device_uninit().
+ */
+ kfree(data);
+ return ret;
+ }
+
+ ret = auxiliary_device_add(&data->adev);
+ if (ret)
+ goto err_did_init;
+
+ client->_opaque = data;
+
+ return 0;
+
+err_did_init:
+ auxiliary_device_uninit(&data->adev);
+ return ret;
+}
+
+/**
+ * dp_aux_unregister_ep_client() - Inverse of dp_aux_register_ep_client()
+ * @client: The structure describing the client.
+ *
+ * If dp_aux_register_ep_client() returns no error then you should call this
+ * to free resources.
+ */
+void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client)
+{
+ struct dp_aux_ep_client_data *data = client->_opaque;
+
+ auxiliary_device_delete(&data->adev);
+ auxiliary_device_uninit(&data->adev);
+}
+
+static void dp_aux_unregister_ep_client_void(void *data)
+{
+ dp_aux_unregister_ep_client(data);
+}
+
+/**
+ * devm_dp_aux_register_ep_client() - devm wrapper for dp_aux_register_ep_client()
+ * @client: The structure describing the client.
+ *
+ * Handles freeing w/ devm on the device "client->aux->dev".
+ *
+ * Return: 0 if no error or negative error code.
+ */
+int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client)
+{
+ int ret;
+
+ ret = dp_aux_register_ep_client(client);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(client->aux->dev,
+ dp_aux_unregister_ep_client_void,
+ client);
+}
+
+static const struct auxiliary_device_id dp_aux_ep_client_id_table[] = {
+ { .name = "drm_dp_aux_bus.ep_client", },
+ {},
+};
+
+static struct auxiliary_driver dp_aux_ep_client_driver = {
+ .name = "ep_client",
+ .probe = dp_aux_ep_client_probe,
+ .remove = dp_aux_ep_client_remove,
+ .shutdown = dp_aux_ep_client_shutdown,
+ .id_table = dp_aux_ep_client_id_table,
+};
+
+/* -----------------------------------------------------------------------------
+ * Module init
+ */
+
static int __init dp_aux_bus_init(void)
{
int ret;
@@ -307,11 +465,16 @@ static int __init dp_aux_bus_init(void)
if (ret)
return ret;
- return 0;
+ ret = auxiliary_driver_register(&dp_aux_ep_client_driver);
+ if (ret)
+ bus_unregister(&dp_aux_bus_type);
+
+ return ret;
}
static void __exit dp_aux_bus_exit(void)
{
+ auxiliary_driver_unregister(&dp_aux_ep_client_driver);
bus_unregister(&dp_aux_bus_type);
}
diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
index 4f19b20b1dd6..ecf68b6873bd 100644
--- a/include/drm/dp/drm_dp_aux_bus.h
+++ b/include/drm/dp/drm_dp_aux_bus.h
@@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
struct module *owner);
void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
+/**
+ * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
+ *
+ * The DP AUX bus can be a bit tricky to use properly. Usually, the way
+ * things work is that:
+ * - The DP controller driver provides the DP AUX bus and would like to probe
+ * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
+ * routine.
+ * - The DP controller driver would also like to acquire a reference to the
+ * DP AUX endpoints (AKA the panel) as part of its probe.
+ *
+ * The problem is that the DP AUX endpoints aren't guaranteed to complete their
+ * probe right away. They could be probing asynchronously or they simply might
+ * fail to acquire some resource and return -EPROBE_DEFER.
+ *
+ * The best way to solve this is to break the DP controller's probe into
+ * two parts. The first part will create the DP AUX bus. The second part will
+ * acquire the reference to the DP AUX endpoint. The first part can complete
+ * finish with no problems and be "done" even if the second part ends up
+ * deferring while waiting for the DP AUX endpoint.
+ *
+ * The dp_aux_ep_client structure and associated functions help with managing
+ * this common case. They will create/add a second "struct device" for you.
+ * In the probe for this second "struct device" (known as the "clientdev" here)
+ * you can acquire references to the AUX DP endpoints and can freely return
+ * -EPROBE_DEFER if they're not ready yet.
+ *
+ * A few notes:
+ * - The parent of the clientdev is guaranteed to be aux->dev
+ * - The of_node of the clientdev is guaranteed to be the same as the of_node
+ * of aux->dev, copied with device_set_of_node_from_dev().
+ * - If you're doing "devm" type things in the clientdev's probe you should
+ * use the clientdev. This makes lifetimes be managed properly.
+ *
+ * NOTE: there's no requirement to use these helpers if you have solved the
+ * problem described above in some other way.
+ */
+struct dp_aux_ep_client {
+ /** @probe: The second half of the probe */
+ int (*probe)(struct device *clientdev, struct dp_aux_ep_client *client);
+
+ /** @remove: Remove function corresponding to the probe */
+ void (*remove)(struct device *clientdev, struct dp_aux_ep_client *client);
+
+ /** @shutdown: Shutdown function corresponding to the probe */
+ void (*shutdown)(struct device *clientdev, struct dp_aux_ep_client *client);
+
+ /** @aux: The AUX bus */
+ struct drm_dp_aux *aux;
+
+ /** @_opaque: Used by the implementation */
+ void *_opaque;
+};
+
+int dp_aux_register_ep_client(struct dp_aux_ep_client *client);
+void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client);
+int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client);
+
#endif /* _DP_AUX_BUS_H_ */
--
2.35.1.1178.g4f1659d476-goog
Sometimes it's useful for users of the DP AUX bus (like panels) to be
able to poll HPD. Let's add a callback that allows DP AUX busses
drivers to provide this.
Suggested-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---
include/drm/dp/drm_dp_helper.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h
index dad1442c91df..a12951319573 100644
--- a/include/drm/dp/drm_dp_helper.h
+++ b/include/drm/dp/drm_dp_helper.h
@@ -2021,6 +2021,20 @@ struct drm_dp_aux {
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
+ /**
+ * @is_hpd_asserted: returns true if HPD is asserted
+ *
+ * This is mainly useful for eDP panels drivers to query whether
+ * an eDP panel has finished powering on. This is an optional function.
+ *
+ * NOTE: this function specifically reports the state of the HPD pin
+ * that's associated with the DP AUX channel. This is different from
+ * the HPD concept in much of the rest of DRM which is more about
+ * physical presence of a display. For eDP, for instance, a display is
+ * assumed always present even if the HPD pin is deasserted.
+ */
+ bool (*is_hpd_asserted)(struct drm_dp_aux *aux);
+
/**
* @i2c_nack_count: Counts I2C NACKs, used for DP validation.
*/
--
2.35.1.1178.g4f1659d476-goog
While it works, for the most part, to assume that the panel has
finished probing when devm_of_dp_aux_populate_ep_devices() returns,
it's a bit fragile. This is talked about at length in commit
a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to
its own sub-dev").
When reviewing the ps8640 code, I managed to convince myself that it
was OK not to worry about it there and that maybe it wasn't really
_that_ fragile. However, it turns out that it really is. Simply
hardcoding panel_edp_probe() to return -EPROBE_DEFER was enough to put
the boot process into an infinite loop. I believe this manages to trip
the same issues that we used to trip with the main MSM code where
something about our actions trigger Linux to re-probe previously
deferred devices right away and each time we try again we re-trigger
Linux to re-probe.
It's really not that crazy to just break the probe up. Let's use the
new helpers introduced in the patch ("drm/dp: Helpers to make it
easier for drivers to use DP AUX bus properly") to break the driver
into two.
With this change, the device still boots (though obviously the panel
doesn't come up) if I force panel-edp to always return -EPROBE_DEFER.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 66 +++++++++++++++-----------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 9766cbbd62ad..96e883985608 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -93,6 +93,7 @@ enum ps8640_vdo_control {
};
struct ps8640 {
+ struct dp_aux_ep_client ep_client;
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
struct drm_dp_aux aux;
@@ -584,10 +585,36 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg
return 0;
}
+static int ps8640_bridge_probe(struct device *clientdev, struct dp_aux_ep_client *client)
+{
+ struct ps8640 *ps_bridge = container_of(client, struct ps8640, ep_client);
+ struct device_node *np = clientdev->of_node;
+ int ret;
+
+ /* port@1 is ps8640 output port */
+ ps_bridge->panel_bridge = devm_drm_of_get_bridge(clientdev, np, 1, 0);
+ if (IS_ERR(ps_bridge->panel_bridge))
+ return PTR_ERR(ps_bridge->panel_bridge);
+
+ drm_bridge_add(&ps_bridge->bridge);
+
+ ret = ps8640_bridge_host_attach(clientdev, ps_bridge);
+ if (ret)
+ drm_bridge_remove(&ps_bridge->bridge);
+
+ return ret;
+}
+
+static void ps8640_bridge_remove(struct device *clientdev, struct dp_aux_ep_client *client)
+{
+ struct ps8640 *ps_bridge = container_of(client, struct ps8640, ep_client);
+
+ drm_bridge_remove(&ps_bridge->bridge);
+}
+
static int ps8640_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
- struct device_node *np = dev->of_node;
struct ps8640 *ps_bridge;
int ret;
u32 i;
@@ -672,31 +699,17 @@ static int ps8640_probe(struct i2c_client *client)
devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
- /* port@1 is ps8640 output port */
- ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0);
- if (IS_ERR(ps_bridge->panel_bridge))
- return PTR_ERR(ps_bridge->panel_bridge);
-
- drm_bridge_add(&ps_bridge->bridge);
-
- ret = ps8640_bridge_host_attach(dev, ps_bridge);
- if (ret)
- goto err_bridge_remove;
-
- return 0;
-
-err_bridge_remove:
- drm_bridge_remove(&ps_bridge->bridge);
- return ret;
-}
-
-static int ps8640_remove(struct i2c_client *client)
-{
- struct ps8640 *ps_bridge = i2c_get_clientdata(client);
-
- drm_bridge_remove(&ps_bridge->bridge);
-
- return 0;
+ /*
+ * Create a sub-device and kick off a probe to it. This effectively
+ * breaks our probe in two and lets the first half complete even if
+ * the second half might return -EPROBE_DEFER. If we didn't do this
+ * then if a panel isn't immediately ready we'd delete it right away
+ * and never give it a chance to finish.
+ */
+ ps_bridge->ep_client.probe = ps8640_bridge_probe;
+ ps_bridge->ep_client.remove = ps8640_bridge_remove;
+ ps_bridge->ep_client.aux = &ps_bridge->aux;
+ return devm_dp_aux_register_ep_client(&ps_bridge->ep_client);
}
static const struct of_device_id ps8640_match[] = {
@@ -707,7 +720,6 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
static struct i2c_driver ps8640_driver = {
.probe_new = ps8640_probe,
- .remove = ps8640_remove,
.driver = {
.name = "ps8640",
.of_match_table = ps8640_match,
--
2.35.1.1178.g4f1659d476-goog
On Fri, 08 Apr 2022, Douglas Anderson <[email protected]> wrote:
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
>
> Signed-off-by: Douglas Anderson <[email protected]>
What is an "EP" client or device?
BR,
Jani.
> ---
>
> drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++++++++++++++++++++++++++-
> include/drm/dp/drm_dp_aux_bus.h | 58 ++++++++++
> 2 files changed, 222 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> index 415afce3cf96..5386ceacf133 100644
> --- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> @@ -12,6 +12,7 @@
> * to perform transactions on that bus.
> */
>
> +#include <linux/auxiliary_bus.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *drv)
> }
> EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
>
> +/* -----------------------------------------------------------------------------
> + * DP AUX EP Client
> + */
> +
> +struct dp_aux_ep_client_data {
> + struct dp_aux_ep_client *client;
> + struct auxiliary_device adev;
> +};
> +
> +static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (!data->client->probe)
> + return 0;
> +
> + return data->client->probe(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->remove)
> + data->client->remove(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->shutdown)
> + data->client->shutdown(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_dev_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + kfree(data);
> +}
> +
> +/**
> + * dp_aux_register_ep_client() - Register an DP AUX EP client
> + * @client: The structure describing the client. It's the client's
> + * responsibility to keep this memory around until
> + * dp_aux_unregister_ep_client() is called, either explicitly or
> + * implicitly via devm.
> + *
> + * See the description of "struct dp_aux_ep_client" for a full explanation
> + * of when you should use this and why.
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->adev.name = "ep_client";
> + data->adev.dev.parent = client->aux->dev;
> + data->adev.dev.release = dp_aux_ep_client_dev_release;
> + device_set_of_node_from_dev(&data->adev.dev, client->aux->dev);
> +
> + ret = auxiliary_device_init(&data->adev);
> + if (ret) {
> + /*
> + * NOTE: if init doesn't fail then it takes ownership
> + * of memory and this kfree() is magically part of
> + * auxiliary_device_uninit().
> + */
> + kfree(data);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(&data->adev);
> + if (ret)
> + goto err_did_init;
> +
> + client->_opaque = data;
> +
> + return 0;
> +
> +err_did_init:
> + auxiliary_device_uninit(&data->adev);
> + return ret;
> +}
> +
> +/**
> + * dp_aux_unregister_ep_client() - Inverse of dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * If dp_aux_register_ep_client() returns no error then you should call this
> + * to free resources.
> + */
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data = client->_opaque;
> +
> + auxiliary_device_delete(&data->adev);
> + auxiliary_device_uninit(&data->adev);
> +}
> +
> +static void dp_aux_unregister_ep_client_void(void *data)
> +{
> + dp_aux_unregister_ep_client(data);
> +}
> +
> +/**
> + * devm_dp_aux_register_ep_client() - devm wrapper for dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * Handles freeing w/ devm on the device "client->aux->dev".
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + int ret;
> +
> + ret = dp_aux_register_ep_client(client);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(client->aux->dev,
> + dp_aux_unregister_ep_client_void,
> + client);
> +}
> +
> +static const struct auxiliary_device_id dp_aux_ep_client_id_table[] = {
> + { .name = "drm_dp_aux_bus.ep_client", },
> + {},
> +};
> +
> +static struct auxiliary_driver dp_aux_ep_client_driver = {
> + .name = "ep_client",
> + .probe = dp_aux_ep_client_probe,
> + .remove = dp_aux_ep_client_remove,
> + .shutdown = dp_aux_ep_client_shutdown,
> + .id_table = dp_aux_ep_client_id_table,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Module init
> + */
> +
> static int __init dp_aux_bus_init(void)
> {
> int ret;
> @@ -307,11 +465,16 @@ static int __init dp_aux_bus_init(void)
> if (ret)
> return ret;
>
> - return 0;
> + ret = auxiliary_driver_register(&dp_aux_ep_client_driver);
> + if (ret)
> + bus_unregister(&dp_aux_bus_type);
> +
> + return ret;
> }
>
> static void __exit dp_aux_bus_exit(void)
> {
> + auxiliary_driver_unregister(&dp_aux_ep_client_driver);
> bus_unregister(&dp_aux_bus_type);
> }
>
> diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
> index 4f19b20b1dd6..ecf68b6873bd 100644
> --- a/include/drm/dp/drm_dp_aux_bus.h
> +++ b/include/drm/dp/drm_dp_aux_bus.h
> @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
> struct module *owner);
> void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
>
> +/**
> + * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
> + *
> + * The DP AUX bus can be a bit tricky to use properly. Usually, the way
> + * things work is that:
> + * - The DP controller driver provides the DP AUX bus and would like to probe
> + * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
> + * routine.
> + * - The DP controller driver would also like to acquire a reference to the
> + * DP AUX endpoints (AKA the panel) as part of its probe.
> + *
> + * The problem is that the DP AUX endpoints aren't guaranteed to complete their
> + * probe right away. They could be probing asynchronously or they simply might
> + * fail to acquire some resource and return -EPROBE_DEFER.
> + *
> + * The best way to solve this is to break the DP controller's probe into
> + * two parts. The first part will create the DP AUX bus. The second part will
> + * acquire the reference to the DP AUX endpoint. The first part can complete
> + * finish with no problems and be "done" even if the second part ends up
> + * deferring while waiting for the DP AUX endpoint.
> + *
> + * The dp_aux_ep_client structure and associated functions help with managing
> + * this common case. They will create/add a second "struct device" for you.
> + * In the probe for this second "struct device" (known as the "clientdev" here)
> + * you can acquire references to the AUX DP endpoints and can freely return
> + * -EPROBE_DEFER if they're not ready yet.
> + *
> + * A few notes:
> + * - The parent of the clientdev is guaranteed to be aux->dev
> + * - The of_node of the clientdev is guaranteed to be the same as the of_node
> + * of aux->dev, copied with device_set_of_node_from_dev().
> + * - If you're doing "devm" type things in the clientdev's probe you should
> + * use the clientdev. This makes lifetimes be managed properly.
> + *
> + * NOTE: there's no requirement to use these helpers if you have solved the
> + * problem described above in some other way.
> + */
> +struct dp_aux_ep_client {
> + /** @probe: The second half of the probe */
> + int (*probe)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @remove: Remove function corresponding to the probe */
> + void (*remove)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @shutdown: Shutdown function corresponding to the probe */
> + void (*shutdown)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @aux: The AUX bus */
> + struct drm_dp_aux *aux;
> +
> + /** @_opaque: Used by the implementation */
> + void *_opaque;
> +};
> +
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client);
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +
> #endif /* _DP_AUX_BUS_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Hi,
On Mon, Apr 11, 2022 at 1:34 AM Jani Nikula <[email protected]> wrote:
>
> On Fri, 08 Apr 2022, Douglas Anderson <[email protected]> wrote:
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
> >
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> What is an "EP" client or device?
The DP AUX EndPoint (or DP AUX EP) is just the generic name I called
the thing on the other side of the DP AUX bus, AKA the "panel".
The "DP AUX EP client" (ep_client) is the code that needs a reference
to the panel.
I'll beef up the patch description and the comments around the
structure to try to make this clearer.
-Doug
Let's add support for being able to read the HPD pin even if it's
hooked directly to the controller. This will let us take away the
waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 35 +++++++++++++++----
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index 20666b6217e7..f72bdd7ff7a1 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -30,6 +30,7 @@ struct atana33xc20_panel {
struct regulator *supply;
struct gpio_desc *el_on3_gpio;
+ struct drm_dp_aux *aux;
struct edid *edid;
@@ -76,6 +77,19 @@ static int atana33xc20_suspend(struct device *dev)
return 0;
}
+static bool atana33xc20_can_read_hpd(struct atana33xc20_panel *p)
+{
+ return !p->no_hpd && (p->hpd_gpio || p->aux->is_hpd_asserted);
+}
+
+static bool panel_edp_read_hpd(struct atana33xc20_panel *p)
+{
+ if (p->hpd_gpio)
+ return gpiod_get_value_cansleep(p->hpd_gpio);
+
+ return p->aux->is_hpd_asserted(p->aux);
+}
+
static int atana33xc20_resume(struct device *dev)
{
struct atana33xc20_panel *p = dev_get_drvdata(dev);
@@ -92,17 +106,24 @@ static int atana33xc20_resume(struct device *dev)
/*
* Handle HPD. Note: if HPD is hooked up to a dedicated pin on the
- * eDP controller then "no_hpd" will be false _and_ "hpd_gpio" will be
- * NULL. It's up to the controller driver to wait for HPD after
- * preparing the panel in that case.
+ * eDP controller then it's possible that "no_hpd" will be false _and_
+ * atana33xc20_can_read_hpd() will return false. It's up to the
+ * controller driver to wait for HPD after preparing the panel in that
+ * case.
*/
if (p->no_hpd) {
/* T3 VCC to HPD high is max 200 ms */
msleep(200);
- } else if (p->hpd_gpio) {
- ret = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
+ } else if (atana33xc20_can_read_hpd(p)) {
+ /*
+ * Even though max HPD is 200 ms, we give an extra long timeout
+ * of 500 ms here. Why? ps8640 is crazy and the bridge chip
+ * driver itself has over 200 ms of delay if it needs to
+ * do the pm_runtime resume of the bridge chip to read the HPD.
+ */
+ ret = readx_poll_timeout(panel_edp_read_hpd, p,
hpd_asserted, hpd_asserted,
- 1000, 200000);
+ 1000, 500000);
if (!hpd_asserted)
dev_warn(dev, "Timeout waiting for HPD\n");
}
@@ -263,6 +284,8 @@ static int atana33xc20_probe(struct dp_aux_ep_device *aux_ep)
return -ENOMEM;
dev_set_drvdata(dev, panel);
+ panel->aux = aux_ep->aux;
+
panel->supply = devm_regulator_get(dev, "power");
if (IS_ERR(panel->supply))
return dev_err_probe(dev, PTR_ERR(panel->supply),
--
2.35.1.1178.g4f1659d476-goog
On 09/04/2022 05:36, Douglas Anderson wrote:
> Sometimes it's useful for users of the DP AUX bus (like panels) to be
> able to poll HPD. Let's add a callback that allows DP AUX busses
> drivers to provide this.
>
> Suggested-by: Dmitry Baryshkov <[email protected]>
> Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
>
> include/drm/dp/drm_dp_helper.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h
> index dad1442c91df..a12951319573 100644
> --- a/include/drm/dp/drm_dp_helper.h
> +++ b/include/drm/dp/drm_dp_helper.h
> @@ -2021,6 +2021,20 @@ struct drm_dp_aux {
> ssize_t (*transfer)(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg);
>
> + /**
> + * @is_hpd_asserted: returns true if HPD is asserted
> + *
> + * This is mainly useful for eDP panels drivers to query whether
> + * an eDP panel has finished powering on. This is an optional function.
> + *
> + * NOTE: this function specifically reports the state of the HPD pin
> + * that's associated with the DP AUX channel. This is different from
> + * the HPD concept in much of the rest of DRM which is more about
> + * physical presence of a display. For eDP, for instance, a display is
> + * assumed always present even if the HPD pin is deasserted.
> + */
> + bool (*is_hpd_asserted)(struct drm_dp_aux *aux);
> +
> /**
> * @i2c_nack_count: Counts I2C NACKs, used for DP validation.
> */
--
With best wishes
Dmitry
On 09/04/2022 05:36, Douglas Anderson wrote:
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
I have been thinking about your approach for quite some time. I think
that enforcing a use of auxilliary device is an overkill. What do we
really need is the the set callbacks in the bus struct or a notifier. We
have to notify the aux_bus controller side that the client has been
probed successfully or that the client is going to be removed. And this
approach would make driver's life easier, since e.g. the bus code can
pm_get the EP device before calling callbacks/notifiers and
pm_put_autosuspend it afterwards.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++++++++++++++++++++++++++-
> include/drm/dp/drm_dp_aux_bus.h | 58 ++++++++++
> 2 files changed, 222 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> index 415afce3cf96..5386ceacf133 100644
> --- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> @@ -12,6 +12,7 @@
> * to perform transactions on that bus.
> */
>
> +#include <linux/auxiliary_bus.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *drv)
> }
> EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
>
> +/* -----------------------------------------------------------------------------
> + * DP AUX EP Client
> + */
> +
> +struct dp_aux_ep_client_data {
> + struct dp_aux_ep_client *client;
> + struct auxiliary_device adev;
> +};
> +
> +static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (!data->client->probe)
> + return 0;
> +
> + return data->client->probe(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->remove)
> + data->client->remove(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->shutdown)
> + data->client->shutdown(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_dev_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + kfree(data);
> +}
> +
> +/**
> + * dp_aux_register_ep_client() - Register an DP AUX EP client
> + * @client: The structure describing the client. It's the client's
> + * responsibility to keep this memory around until
> + * dp_aux_unregister_ep_client() is called, either explicitly or
> + * implicitly via devm.
> + *
> + * See the description of "struct dp_aux_ep_client" for a full explanation
> + * of when you should use this and why.
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->adev.name = "ep_client";
> + data->adev.dev.parent = client->aux->dev;
> + data->adev.dev.release = dp_aux_ep_client_dev_release;
> + device_set_of_node_from_dev(&data->adev.dev, client->aux->dev);
> +
> + ret = auxiliary_device_init(&data->adev);
> + if (ret) {
> + /*
> + * NOTE: if init doesn't fail then it takes ownership
> + * of memory and this kfree() is magically part of
> + * auxiliary_device_uninit().
> + */
> + kfree(data);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(&data->adev);
> + if (ret)
> + goto err_did_init;
> +
> + client->_opaque = data;
> +
> + return 0;
> +
> +err_did_init:
> + auxiliary_device_uninit(&data->adev);
> + return ret;
> +}
> +
> +/**
> + * dp_aux_unregister_ep_client() - Inverse of dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * If dp_aux_register_ep_client() returns no error then you should call this
> + * to free resources.
> + */
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data = client->_opaque;
> +
> + auxiliary_device_delete(&data->adev);
> + auxiliary_device_uninit(&data->adev);
> +}
> +
> +static void dp_aux_unregister_ep_client_void(void *data)
> +{
> + dp_aux_unregister_ep_client(data);
> +}
> +
> +/**
> + * devm_dp_aux_register_ep_client() - devm wrapper for dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * Handles freeing w/ devm on the device "client->aux->dev".
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + int ret;
> +
> + ret = dp_aux_register_ep_client(client);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(client->aux->dev,
> + dp_aux_unregister_ep_client_void,
> + client);
> +}
> +
> +static const struct auxiliary_device_id dp_aux_ep_client_id_table[] = {
> + { .name = "drm_dp_aux_bus.ep_client", },
> + {},
> +};
> +
> +static struct auxiliary_driver dp_aux_ep_client_driver = {
> + .name = "ep_client",
> + .probe = dp_aux_ep_client_probe,
> + .remove = dp_aux_ep_client_remove,
> + .shutdown = dp_aux_ep_client_shutdown,
> + .id_table = dp_aux_ep_client_id_table,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Module init
> + */
> +
> static int __init dp_aux_bus_init(void)
> {
> int ret;
> @@ -307,11 +465,16 @@ static int __init dp_aux_bus_init(void)
> if (ret)
> return ret;
>
> - return 0;
> + ret = auxiliary_driver_register(&dp_aux_ep_client_driver);
> + if (ret)
> + bus_unregister(&dp_aux_bus_type);
> +
> + return ret;
> }
>
> static void __exit dp_aux_bus_exit(void)
> {
> + auxiliary_driver_unregister(&dp_aux_ep_client_driver);
> bus_unregister(&dp_aux_bus_type);
> }
>
> diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
> index 4f19b20b1dd6..ecf68b6873bd 100644
> --- a/include/drm/dp/drm_dp_aux_bus.h
> +++ b/include/drm/dp/drm_dp_aux_bus.h
> @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
> struct module *owner);
> void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
>
> +/**
> + * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
> + *
> + * The DP AUX bus can be a bit tricky to use properly. Usually, the way
> + * things work is that:
> + * - The DP controller driver provides the DP AUX bus and would like to probe
> + * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
> + * routine.
> + * - The DP controller driver would also like to acquire a reference to the
> + * DP AUX endpoints (AKA the panel) as part of its probe.
> + *
> + * The problem is that the DP AUX endpoints aren't guaranteed to complete their
> + * probe right away. They could be probing asynchronously or they simply might
> + * fail to acquire some resource and return -EPROBE_DEFER.
> + *
> + * The best way to solve this is to break the DP controller's probe into
> + * two parts. The first part will create the DP AUX bus. The second part will
> + * acquire the reference to the DP AUX endpoint. The first part can complete
> + * finish with no problems and be "done" even if the second part ends up
> + * deferring while waiting for the DP AUX endpoint.
> + *
> + * The dp_aux_ep_client structure and associated functions help with managing
> + * this common case. They will create/add a second "struct device" for you.
> + * In the probe for this second "struct device" (known as the "clientdev" here)
> + * you can acquire references to the AUX DP endpoints and can freely return
> + * -EPROBE_DEFER if they're not ready yet.
> + *
> + * A few notes:
> + * - The parent of the clientdev is guaranteed to be aux->dev
> + * - The of_node of the clientdev is guaranteed to be the same as the of_node
> + * of aux->dev, copied with device_set_of_node_from_dev().
> + * - If you're doing "devm" type things in the clientdev's probe you should
> + * use the clientdev. This makes lifetimes be managed properly.
> + *
> + * NOTE: there's no requirement to use these helpers if you have solved the
> + * problem described above in some other way.
> + */
> +struct dp_aux_ep_client {
> + /** @probe: The second half of the probe */
> + int (*probe)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @remove: Remove function corresponding to the probe */
> + void (*remove)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @shutdown: Shutdown function corresponding to the probe */
> + void (*shutdown)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @aux: The AUX bus */
> + struct drm_dp_aux *aux;
> +
> + /** @_opaque: Used by the implementation */
> + void *_opaque;
> +};
> +
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client);
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +
> #endif /* _DP_AUX_BUS_H_ */
--
With best wishes
Dmitry
On Sat, 16 Apr 2022 at 00:13, Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
> <[email protected]> wrote:
> >
> > On 09/04/2022 05:36, Douglas Anderson wrote:
> > > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > > DP AUX bus properly we really need two "struct device"s. One "struct
> > > device" is in charge of providing the DP AUX bus and the other is
> > > where we'll try to get a reference to the newly probed endpoint
> > > devices.
> > >
> > > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > > is already broken up into several "struct devices" anyway because it
> > > also provides a PWM and some GPIOs. Adding one more wasn't that
> > > difficult / ugly.
> > >
> > > When I tried to do the same solution in parade-ps8640, it felt like I
> > > was copying too much boilerplate code. I made the realization that I
> > > didn't _really_ need a separate "driver" for each person that wanted
> > > to do the same thing. By putting all the "driver" related code in a
> > > common place then we could save a bit of hassle. This change
> > > effectively adds a new "ep_client" driver that can be used by
> > > anyone. The devices instantiated by this driver will just call through
> > > to the probe/remove/shutdown calls provided.
> > >
> > > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > > want to expose this to clients, though, so as far as clients are
> > > concerned they get a vanilla "struct device".
> >
> > I have been thinking about your approach for quite some time. I think
> > that enforcing a use of auxilliary device is an overkill. What do we
> > really need is the the set callbacks in the bus struct or a notifier. We
> > have to notify the aux_bus controller side that the client has been
> > probed successfully or that the client is going to be removed.
>
> It seems like these new callbacks would be nearly the same as the
> probe/remove callbacks in my proposal except:
>
> * They rely on there being exactly 1 AUX device, or we make it a rule
> that we wait for all AUX devices to probe (?)
Is the backlight a separate device on an AUX bus? Judging from
drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
a point-to-point bus, so there is always a single client.
>
> * We need to come up with a system for detecting when everything
> probes or is going to be removed, though that's probably not too hard.
> I guess the DP AUX bus could just replace the panel's probe function
> with its own and essentially "tail patch" it. I guess it could "head
> patch" the remove call? ...or is there some better way you were
> thinking of knowing when all our children probed?
>
> * The callback on the aux bus controller side would not be able to
> DEFER. In other words trying to acquire a reference to the panel can
> always be the last thing we do so we know there can be no reasons to
> defer after. This should be doable, but at least in the ps8640 case it
> will require changing the code a bit. I notice that today it actually
> tries to get the panel side _before_ it gets the MIPI side and it
> potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> guess I have a niggling feeling that we'll find some reason in the
> future that we can't be last, but we can probably ignore that. ;-)
>
> I can switch this all to normal callbacks if that's what everyone
> wants, but it doesn't feel significantly cleaner to me and does seem
> to have some (small) downsides.
>
>
> > And this
> > approach would make driver's life easier, since e.g. the bus code can
> > pm_get the EP device before calling callbacks/notifiers and
> > pm_put_autosuspend it afterwards.
>
> Not sure about doing the pm calls on behalf of the EP device. What's
> the goal there?
I think any driver can pm_runtime_get another device. The goal is to
let the 'post_probe' callback to power up the panel, read the EDID,
etc.
BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
write the EDID checksum like we do for DP?
Do you have any good summary for eDP vs DP differences?
--
With best wishes
Dmitry
Hi,
On Thu, Apr 14, 2022 at 4:52 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Douglas Anderson (2022-04-08 19:36:23)
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
> >
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
>
> Is it correct that the struct dp_aux_ep_client is largely equivalent to
> a struct dp_aux_ep_device? What's the difference? I think it is a fully
> probed dp_aux_ep_device? Or a way to tell the driver that calls
> of_dp_aux_populate_ep_devices() that the endpoint has probed now?
They're not the same. The "DP AUX Endpoint Device" is essentially the
panel, though at one point in time someone argued that conceivably one
could put other devices on it even though this might be a really bad
idea. At some point in the discussion someone mentioned the concept of
a touchscreen running over DP Aux had been discussed and, indeed, "dp
aux touchscreen" gets hits if you search for it. The idea that it
could be something different is one reason why I refrained from
calling it a panel in all the function names and always tried to call
it a "DP AUX Endpoint".
The "DP AUX Endpoint Device Client" is the client of the panel, or the
code that needs to get a reference to the panel. Logically, I guess
this is the part of the eDP controller that's responsible for shoving
bits to the panel. Essentially the drm_bridge. Most importantly, it's
_not_ the part of the eDP controller providing the AUX bus.
> I read the commit text but it didn't click until I read the next patch
> that this is solving a probe ordering/loop problem. The driver that
> creates the 'struct drm_dp_aux' and populates devices on the DP aux bus
> with of_dp_aux_populate_ep_devices() wants to be sure that the
> drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in
> edp-panel is going to be there before calling drm_of_get_bridge().
>
> of_dp_aux_populate_ep_devices();
> [No idea if the bridge is registered yet]
> drm_of_get_bridge();
>
> The solution is to retry the drm_of_get_bridge() until 'struct
> dp_aux_ep_driver' probes and registers the next bridge. It looks like a
> wait_for_completion() on top of drm_of_get_bridge() implemented through
> driver probe and -EPROBE_DEFER; no disrespect!
Yes, that's exactly what it is.
> Is there another problem here that the driver that creates the 'struct
> drm_dp_aux' and populates devices on the DP aux bus with
> of_dp_aux_populate_ep_devices() wants to use that same 'struct
> drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was
> populated? And the 'struct dp_aux_ep_device' may either not be probed
> and bound to a 'struct dp_aux_ep_driver' or it may not be powered on
> because it went to runtime PM suspend?
>
> Put another way, I worry that the owner of 'struct drm_dp_aux' wants to
> use some function in include/drm/dp/drm_dp_helper.h that takes the
> 'struct drm_dp_aux' directly without considering the device model state
> of the endpoint device (the 'struct dp_aux_ep_device'). That would be a
> similar problem as waiting for the endpoint to be powered on and probed.
> Solving that through a sub-driver feels awkward.
>
> What if we had some function like drm_dp_get_aux_ep() that took a
> 'struct drm_dp_aux' and looked for the endpoint device (there can only
> be one?),
The code is designed to handle the fact that there could be more than
one AUX endpoint device. I don't know if this will ever happen but it
is plausible. The "touchscreen over DP AUX", if that ever became a
thing supported in Linux, could be an example. In some sense, I guess
we could have modeled the AUX backlight for homestar this way as a
second "DP AUX Endpoint", though we didn't...
> waited for it to be probed,
The $1M question: where should it be doing the waiting? Are you
imagining this straight in my probe? AKA:
def ps8640_probe(...):
...
devm_of_dp_aux_populate_ep_devices(...);
do_the_wait(...);
bridge = devm_drm_of_get_bridge(...);
if (bridge == -EPROBE_DEFER)
return -EPROBE_DEFER;
...
Essentially, if the panel is probing asynchronously then this would
wait for it. If the panel instead needs some resources then it should
be fine for us to just pass the -EPROBE_DEFER up and we'll both try
probing again later. That definitely feels simpler to me and matches
how I would wish things to work.
There are two problems:
1. I'm not sure how to wait. Early in ps8640 support I had Philip try
wait_for_device_probe(). That wasn't so happy. I suppose I could try
to come up with some solution if this is indeed the way we want to go.
2. The way probing currently works, if we end up in the -EPROBE_DEFER
case then we end up with an infinite loop. :( As I understand it, the
basic rule is that if your probe causes any additional devices to be
created (like by calling devm_of_dp_aux_populate_ep_devices()) then
your probe isn't allowed to return -EPROBE_DEFER after the call. This
is the same problem that the main msm used to have (Dmitry says it's
fixed now). I think this is just a fundamental design issue with
deferred probing. Anytime a device probes then it causes
driver_deferred_probe_trigger() to run again. So we basically have:
a) ps8640 probe returns -EPROBE_DEFER for whatever reason.
b) some device in the system probes and all deferred probes are retriggered.
c) ps8640 gets pulled off the list by the worker
d) ps8640 probe gets called
e) ps8640 creates a sub device for the panel, which triggers deferred probing
f) ps8640 returns and gets put on the deferred list
g) goto step c)
On first instinct, you might think this is easy to solve. Maybe
somehow you could make it so the "trigger" doesn't re-trigger the
ps8640 since it hasn't actually _returned_ from probing yet. The
problem is that I believe there'd be a race in the async probe case.
See the description for driver_deferred_probe_trigger() for some
details.
What we _could_ do is that we could re-create deferred probing
ourselves. :-P So instead of creating a sub-device, I could kick off
some type of asynchronous task that would periodically run and look to
see if the panel has shown up. Then, once the panel shows up then I
could create the bridge. I'm not really convinced that this is better,
though.
> and then powered it up via some
> pm_runtime_get_sync()? My understanding is that with edp-panel we have
> two power "domains" of importance, the panel power domain and the DP/eDP
> power domain. Access to the AUX bus via 'struct drm_dp_aux' needs both
> power domains to be powered on. If the 'struct dp_aux_ep_device' is in
> hand, then both power domains can be powered on by making sure the power
> state of the 'struct dp_aux_ep_device::dev' is enabled. If only the
> 'struct drm_dp_aux' is in hand then we'll need to do something more like
> find the child device and power it on.
So right now it doesn't work that way. The whole thing is structured
more like an i2c bus. The i2c bus doesn't power its devices on. The
devices are in charge of powering themselves on. If the i2c bus itself
has a low power state that it can be in when the devices don't need to
communicate then that's fine. It can power itself on whenever the
devices need to communicate. If this is a high-cost thing then the bus
can use pm_runtime. Following this model is what leads us to the panel
being in charge of reading the EDID.
> Could the 'struct drm_dp_aux' functions in drm_dp_helper.h do this
> automatically somehow? Maybe we can drop a variable in 'struct
> drm_dp_aux' when of_dp_aux_populate_ep_devices() is called that the
> other side may not be powered up. Then if something tries to transfer on
> that aux channel it powers on all children of 'struct drm_dp_aux::dev'
> that are on the 'dp_aux_bus_type' or bails out if those devices haven't
> probed yet or can't be powered on.
We can have more discussions about powering and who's in charge of
powering who if we need to, but it's not really what this series is
about. Here we're worried about making sure that we acquire all of our
resources before we create the drm_bridge. Said another way: we're
trying to acquire a handle to the panel before we add the bridge
because once we add the bridge we're asserting that we're all ready to
go and start using things, right? So we basically just want to make
sure that the panel is present and able to acquire all of _its_
resources.
-Doug
Quoting Douglas Anderson (2022-04-08 19:36:23)
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
Is it correct that the struct dp_aux_ep_client is largely equivalent to
a struct dp_aux_ep_device? What's the difference? I think it is a fully
probed dp_aux_ep_device? Or a way to tell the driver that calls
of_dp_aux_populate_ep_devices() that the endpoint has probed now?
I read the commit text but it didn't click until I read the next patch
that this is solving a probe ordering/loop problem. The driver that
creates the 'struct drm_dp_aux' and populates devices on the DP aux bus
with of_dp_aux_populate_ep_devices() wants to be sure that the
drm_bridge made by the 'struct dp_aux_ep_driver' probe routine in
edp-panel is going to be there before calling drm_of_get_bridge().
of_dp_aux_populate_ep_devices();
[No idea if the bridge is registered yet]
drm_of_get_bridge();
The solution is to retry the drm_of_get_bridge() until 'struct
dp_aux_ep_driver' probes and registers the next bridge. It looks like a
wait_for_completion() on top of drm_of_get_bridge() implemented through
driver probe and -EPROBE_DEFER; no disrespect!
Is there another problem here that the driver that creates the 'struct
drm_dp_aux' and populates devices on the DP aux bus with
of_dp_aux_populate_ep_devices() wants to use that same 'struct
drm_dp_aux' directly to poke at some 'struct dp_aux_ep_device' that was
populated? And the 'struct dp_aux_ep_device' may either not be probed
and bound to a 'struct dp_aux_ep_driver' or it may not be powered on
because it went to runtime PM suspend?
Put another way, I worry that the owner of 'struct drm_dp_aux' wants to
use some function in include/drm/dp/drm_dp_helper.h that takes the
'struct drm_dp_aux' directly without considering the device model state
of the endpoint device (the 'struct dp_aux_ep_device'). That would be a
similar problem as waiting for the endpoint to be powered on and probed.
Solving that through a sub-driver feels awkward.
What if we had some function like drm_dp_get_aux_ep() that took a
'struct drm_dp_aux' and looked for the endpoint device (there can only
be one?), waited for it to be probed, and then powered it up via some
pm_runtime_get_sync()? My understanding is that with edp-panel we have
two power "domains" of importance, the panel power domain and the DP/eDP
power domain. Access to the AUX bus via 'struct drm_dp_aux' needs both
power domains to be powered on. If the 'struct dp_aux_ep_device' is in
hand, then both power domains can be powered on by making sure the power
state of the 'struct dp_aux_ep_device::dev' is enabled. If only the
'struct drm_dp_aux' is in hand then we'll need to do something more like
find the child device and power it on.
Could the 'struct drm_dp_aux' functions in drm_dp_helper.h do this
automatically somehow? Maybe we can drop a variable in 'struct
drm_dp_aux' when of_dp_aux_populate_ep_devices() is called that the
other side may not be powered up. Then if something tries to transfer on
that aux channel it powers on all children of 'struct drm_dp_aux::dev'
that are on the 'dp_aux_bus_type' or bails out if those devices haven't
probed yet or can't be powered on.
> diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
> index 4f19b20b1dd6..ecf68b6873bd 100644
> --- a/include/drm/dp/drm_dp_aux_bus.h
> +++ b/include/drm/dp/drm_dp_aux_bus.h
> @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
> struct module *owner);
> void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
>
> +/**
> + * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
dp_aux_ep_client?
> + *
> + * The DP AUX bus can be a bit tricky to use properly. Usually, the way
> + * things work is that:
> + * - The DP controller driver provides the DP AUX bus and would like to probe
> + * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
> + * routine.
> + * - The DP controller driver would also like to acquire a reference to the
> + * DP AUX endpoints (AKA the panel) as part of its probe.
> + *
> + * The problem is that the DP AUX endpoints aren't guaranteed to complete their
> + * probe right away. They could be probing asynchronously or they simply might
> + * fail to acquire some resource and return -EPROBE_DEFER.
> + *
> + * The best way to solve this is to break the DP controller's probe into
> + * two parts. The first part will create the DP AUX bus. The second part will
> + * acquire the reference to the DP AUX endpoint. The first part can complete
> + * finish with no problems and be "done" even if the second part ends up
s/finish//
> + * deferring while waiting for the DP AUX endpoint.
> + *
> + * The dp_aux_ep_client structure and associated functions help with managing
> + * this common case. They will create/add a second "struct device" for you.
create and add a second
> + * In the probe for this second "struct device" (known as the "clientdev" here)
> + * you can acquire references to the AUX DP endpoints and can freely return
> + * -EPROBE_DEFER if they're not ready yet.
> + *
> + * A few notes:
> + * - The parent of the clientdev is guaranteed to be aux->dev
> + * - The of_node of the clientdev is guaranteed to be the same as the of_node
> + * of aux->dev, copied with device_set_of_node_from_dev().
> + * - If you're doing "devm" type things in the clientdev's probe you should
> + * use the clientdev. This makes lifetimes be managed properly.
> + *
> + * NOTE: there's no requirement to use these helpers if you have solved the
> + * problem described above in some other way.
> + */
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 09/04/2022 05:36, Douglas Anderson wrote:
> > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > DP AUX bus properly we really need two "struct device"s. One "struct
> > device" is in charge of providing the DP AUX bus and the other is
> > where we'll try to get a reference to the newly probed endpoint
> > devices.
> >
> > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > is already broken up into several "struct devices" anyway because it
> > also provides a PWM and some GPIOs. Adding one more wasn't that
> > difficult / ugly.
> >
> > When I tried to do the same solution in parade-ps8640, it felt like I
> > was copying too much boilerplate code. I made the realization that I
> > didn't _really_ need a separate "driver" for each person that wanted
> > to do the same thing. By putting all the "driver" related code in a
> > common place then we could save a bit of hassle. This change
> > effectively adds a new "ep_client" driver that can be used by
> > anyone. The devices instantiated by this driver will just call through
> > to the probe/remove/shutdown calls provided.
> >
> > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > want to expose this to clients, though, so as far as clients are
> > concerned they get a vanilla "struct device".
>
> I have been thinking about your approach for quite some time. I think
> that enforcing a use of auxilliary device is an overkill. What do we
> really need is the the set callbacks in the bus struct or a notifier. We
> have to notify the aux_bus controller side that the client has been
> probed successfully or that the client is going to be removed.
It seems like these new callbacks would be nearly the same as the
probe/remove callbacks in my proposal except:
* They rely on there being exactly 1 AUX device, or we make it a rule
that we wait for all AUX devices to probe (?)
* We need to come up with a system for detecting when everything
probes or is going to be removed, though that's probably not too hard.
I guess the DP AUX bus could just replace the panel's probe function
with its own and essentially "tail patch" it. I guess it could "head
patch" the remove call? ...or is there some better way you were
thinking of knowing when all our children probed?
* The callback on the aux bus controller side would not be able to
DEFER. In other words trying to acquire a reference to the panel can
always be the last thing we do so we know there can be no reasons to
defer after. This should be doable, but at least in the ps8640 case it
will require changing the code a bit. I notice that today it actually
tries to get the panel side _before_ it gets the MIPI side and it
potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
guess I have a niggling feeling that we'll find some reason in the
future that we can't be last, but we can probably ignore that. ;-)
I can switch this all to normal callbacks if that's what everyone
wants, but it doesn't feel significantly cleaner to me and does seem
to have some (small) downsides.
> And this
> approach would make driver's life easier, since e.g. the bus code can
> pm_get the EP device before calling callbacks/notifiers and
> pm_put_autosuspend it afterwards.
Not sure about doing the pm calls on behalf of the EP device. What's
the goal there?
Hi,
On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On Sat, 16 Apr 2022 at 00:13, Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
> > <[email protected]> wrote:
> > >
> > > On 09/04/2022 05:36, Douglas Anderson wrote:
> > > > As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> > > > patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> > > > ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> > > > DP AUX bus properly we really need two "struct device"s. One "struct
> > > > device" is in charge of providing the DP AUX bus and the other is
> > > > where we'll try to get a reference to the newly probed endpoint
> > > > devices.
> > > >
> > > > In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> > > > is already broken up into several "struct devices" anyway because it
> > > > also provides a PWM and some GPIOs. Adding one more wasn't that
> > > > difficult / ugly.
> > > >
> > > > When I tried to do the same solution in parade-ps8640, it felt like I
> > > > was copying too much boilerplate code. I made the realization that I
> > > > didn't _really_ need a separate "driver" for each person that wanted
> > > > to do the same thing. By putting all the "driver" related code in a
> > > > common place then we could save a bit of hassle. This change
> > > > effectively adds a new "ep_client" driver that can be used by
> > > > anyone. The devices instantiated by this driver will just call through
> > > > to the probe/remove/shutdown calls provided.
> > > >
> > > > At the moment, the "ep_client" driver is backed by the Linux auxiliary
> > > > bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> > > > want to expose this to clients, though, so as far as clients are
> > > > concerned they get a vanilla "struct device".
> > >
> > > I have been thinking about your approach for quite some time. I think
> > > that enforcing a use of auxilliary device is an overkill. What do we
> > > really need is the the set callbacks in the bus struct or a notifier. We
> > > have to notify the aux_bus controller side that the client has been
> > > probed successfully or that the client is going to be removed.
> >
> > It seems like these new callbacks would be nearly the same as the
> > probe/remove callbacks in my proposal except:
> >
> > * They rely on there being exactly 1 AUX device, or we make it a rule
> > that we wait for all AUX devices to probe (?)
>
> Is the backlight a separate device on an AUX bus? Judging from
> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
> a point-to-point bus, so there is always a single client.
Define "device". ;-)
It's a seperate "struct device" from a Linux point of view since it's
a backlight class device. Certainly it's highly correlated to the
display, but one can conceptually think of them as different devices,
sorta. ;-)
I actually dug a tiny bit more into the whole "touchscreen over aux".
I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
would be modeled, of course.
I guess the summary is that I'm OK w/ changing it to assume one device
for now, but I'm still not sure it's compelling to move to normal
callbacks. The API for callbacks is pretty much the same as the one I
proposed and IMO leaving it the way it is (with an extra struct
device) doesn't really add much complexity and has a few (small) nice
benefits.
> > * We need to come up with a system for detecting when everything
> > probes or is going to be removed, though that's probably not too hard.
> > I guess the DP AUX bus could just replace the panel's probe function
> > with its own and essentially "tail patch" it. I guess it could "head
> > patch" the remove call? ...or is there some better way you were
> > thinking of knowing when all our children probed?
> >
> > * The callback on the aux bus controller side would not be able to
> > DEFER. In other words trying to acquire a reference to the panel can
> > always be the last thing we do so we know there can be no reasons to
> > defer after. This should be doable, but at least in the ps8640 case it
> > will require changing the code a bit. I notice that today it actually
> > tries to get the panel side _before_ it gets the MIPI side and it
> > potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> > guess I have a niggling feeling that we'll find some reason in the
> > future that we can't be last, but we can probably ignore that. ;-)
> >
> > I can switch this all to normal callbacks if that's what everyone
> > wants, but it doesn't feel significantly cleaner to me and does seem
> > to have some (small) downsides.
> >
> >
> > > And this
> > > approach would make driver's life easier, since e.g. the bus code can
> > > pm_get the EP device before calling callbacks/notifiers and
> > > pm_put_autosuspend it afterwards.
> >
> > Not sure about doing the pm calls on behalf of the EP device. What's
> > the goal there?
>
> I think any driver can pm_runtime_get another device. The goal is to
> let the 'post_probe' callback to power up the panel, read the EDID,
> etc.
Right. I was hoping to keep this as a separate discussion since I
think it's largely unrelated to the probe ordering issue, but we can
talk about it here if you want.
There are a lot of open questions here and it's definitely hard to
wrap your head around all of it. Maybe I'll just spam some thoughts
and see if they all make sense together...
1. At the moment, there's no guarantee that a DP AUX Endpoint (AKA
panel) will use pm_runtime() to power itself up enough to do an AUX
transfer. At the moment the two eDP panels drivers I'm aware of use
pm_runtime, but that's actually a fairly new behavior. I guess we'd
have to codify it as "required" if we were going to rely on it.
2. In general, panels have powered themselves enough to read the EDID
in their prepare() stage, which is equivalent to the bridge's
pre_enable(). During some of my early patches to try to support EDID
reading in ti-sn65dsi86 I actually relied upon it. It was like that in
v3 [1]. Personally I see this as the "official" interface to power on
the panel from the DP controller. As such I'm not sure we need to add
pm_runtime() as an equivalent option.
3. In the cover letter of v4 of my ti-sn65dsi86 EDID patch series I
talked about why I switched to having EDID reading driven by the panel
instead of powering on the panel (via pre_enable) and reading the EDID
in the controller. One reason talked about there is that the "generic"
eDP panel driver actually needs the EDID, or at least enough of it to
get the panel ID, so that it can adjust its power sequence timings. If
the EDID reading is completely handled by the DP driver and the panel
can't do it then we'd need to figure out how to communicate it back.
4. In general, panels can be pretty persnickety about their power
sequencing. As far as I've been able to tell, the official spec
provides two things you can do:
4a) You can power the panel up enough to do AUX transfers and then
power it back off.
4b) You can power the panel up enough to do AUX transfers, then finish
powering it all the way up (turn on screen, backlight, etc). When you
turn the screen off, if you follow the spec strictly, you're also
_required_ to fully power the panel off. In other words, remove _all_
power from the display including any power that would be needed to do
AUX transfers.
Now the generic eDP panel code doesn't currently follow the
"strict"ness of the spec and I'm not actually sure if that's how the
spec is intended to be interpreted anyway. There are two timing
diagrams, though. One for "aux transfer only" and the other for
"normal system operation". In the "normal system operation" the
diagram doesn't allow for the backlight to ever go off and on again.
Now, despite the fact that the generic eDP panel code doesn't follow
the "strict"ness I just described, the _other_ DP panel I worked on
recently (samsung-atna33xc20) does. In testing we found that this
panel would sometimes (like 1 in 20 times?) crash if you ever stopped
outputting data to the display and then started again. You absolutely
needed to fully power cycle the display each time. I tried to document
this to the best of my ability in atana33xc20_unprepare(). There's
also a WARN_ON() in atana33xc20_enable() trying to detect if someone
is doing something the panel driver doesn't expect. I've also been
trying to keep my eyes out to see if we need to do the same thing in
generic eDP panel code, either for everyone or via some type of
per-panel quirk. There's definitely a good reason to avoid the extra
cycling if possible since powering panels off and on again often
requires hundreds of milliseconds of delay in order to meet timing
diagrams. ...and we do this if we ever change panel "modes".
...OK, so why does this all matter? I guess my point here is I worry a
little bit about saying that the DP controller code can willy nilly
request the panel to be powered whenever it wants. If the DP
controller was trying to hold the panel powered and then we _needed_
to power the panel off then that would be bad. It doesn't mean we
can't be careful about it, of course...
Said another way, in my mental model these three sequences are allowed:
s1) prepare, unprepare
s2) prepare, enable, disable, unprepare
s3) prepare, enable, disable, unprepare, prepare, enable, disable, unprepare
...and this sequence is _not_ allowed:
s4) prepare, enable, disable, enable, disable, unprepare
...and, in my mind, it's up to the panel driver to know whether in
sequence s3) it has to _force_ power off between the unprepare and a
prepare.
If pm_runtime() officially replaces prepare/unprepare then it's less
obvious (in my mind) that we have to coordinate with enable().
5. In general I've been asserting that it should be up to the panel to
power things on and drive all AUX transactions. ...but clearly my
model isn't reality. We certainly do AUX transactions from the DP
driver because the DP driver needs to know things about the connected
device, like the number of lanes it has, the version of eDP it
supports, and the available bit rates to name a few. Those things all
work today by relying on the fact that pre-enable powers the panel on.
It's pretty easy to say that reading the EDID (and I guess AUX
backlight) is the odd one out. So right now I guess my model is:
5a) If the panel code wants to access the AUX bus it can do so by
powering itself on and then just doing an AUX transaction and assuming
that the provider of the AUX bus can power itself on as needed.
5b) If the DP code wants to access the AUX bus it should make sure
that the next bridge's pre_enable() has been called. It can then
assume that the device is powered on until the next bridge's
post_disable() has been called.
So I guess tl;dr: I'm not really a huge fan of the DP driver powering
the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
with the interface that we have to pre_enable() the panel to turn it
on.
[1] https://lore.kernel.org/r/20210402152701.v3.8.Ied721dc895156046ac523baa55a71da241cd09c7@changeid/
[2] https://lore.kernel.org/r/[email protected]/
> BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
> write the EDID checksum like we do for DP?
Write the EDID checksum? I don't know what that means. You mean
dp_panel_get_edid_checksum()? I'm not 100% sure, a quick glance seems
to make me feel it has to do with DP compliance testing? I can dig
more if need be. The generic EDID reading code already calculates the
checksum, so unless you're doing some funny business you shouldn't
need to check it again...
> Do you have any good summary for eDP vs DP differences?
I don't. :( Mostly stuff here is me trying to grok bits out of what
existing drivers were doing and trying to cross reference it with the
eDP spec that I have (which I don't believe I can share,
unfortunately).
-Doug
On 16/04/2022 03:09, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>> On Sat, 16 Apr 2022 at 00:13, Doug Anderson <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov
>>> <[email protected]> wrote:
>>>>
>>>> On 09/04/2022 05:36, Douglas Anderson wrote:
>>>>> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
>>>>> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
>>>>> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
>>>>> DP AUX bus properly we really need two "struct device"s. One "struct
>>>>> device" is in charge of providing the DP AUX bus and the other is
>>>>> where we'll try to get a reference to the newly probed endpoint
>>>>> devices.
>>>>>
>>>>> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
>>>>> is already broken up into several "struct devices" anyway because it
>>>>> also provides a PWM and some GPIOs. Adding one more wasn't that
>>>>> difficult / ugly.
>>>>>
>>>>> When I tried to do the same solution in parade-ps8640, it felt like I
>>>>> was copying too much boilerplate code. I made the realization that I
>>>>> didn't _really_ need a separate "driver" for each person that wanted
>>>>> to do the same thing. By putting all the "driver" related code in a
>>>>> common place then we could save a bit of hassle. This change
>>>>> effectively adds a new "ep_client" driver that can be used by
>>>>> anyone. The devices instantiated by this driver will just call through
>>>>> to the probe/remove/shutdown calls provided.
>>>>>
>>>>> At the moment, the "ep_client" driver is backed by the Linux auxiliary
>>>>> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
>>>>> want to expose this to clients, though, so as far as clients are
>>>>> concerned they get a vanilla "struct device".
>>>>
>>>> I have been thinking about your approach for quite some time. I think
>>>> that enforcing a use of auxilliary device is an overkill. What do we
>>>> really need is the the set callbacks in the bus struct or a notifier. We
>>>> have to notify the aux_bus controller side that the client has been
>>>> probed successfully or that the client is going to be removed.
>>>
>>> It seems like these new callbacks would be nearly the same as the
>>> probe/remove callbacks in my proposal except:
>>>
>>> * They rely on there being exactly 1 AUX device, or we make it a rule
>>> that we wait for all AUX devices to probe (?)
>>
>> Is the backlight a separate device on an AUX bus? Judging from
>> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
>> a point-to-point bus, so there is always a single client.
>
> Define "device". ;-)
"a device on the AUX bus" = the device, which lists dp_aux_bus_type as
dev->bus_type.
>
> It's a seperate "struct device" from a Linux point of view since it's
> a backlight class device. Certainly it's highly correlated to the
> display, but one can conceptually think of them as different devices,
> sorta. ;-)
>
> I actually dug a tiny bit more into the whole "touchscreen over aux".
> I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
> would be modeled, of course.
Ugh. Do you have any details of the standard itself? Like how does it
looks like from the host point of view. And if the AUX is required to be
powered for this USB bus to work?
In other words: if we were to model it at this moment, would it be the
child of the panel device (like backlight) or a separate device sitting
on the same AUX bus?
>
> I guess the summary is that I'm OK w/ changing it to assume one device
> for now, but I'm still not sure it's compelling to move to normal
> callbacks. The API for callbacks is pretty much the same as the one I
> proposed and IMO leaving it the way it is (with an extra struct
> device) doesn't really add much complexity and has a few (small) nice
> benefits.
I think Stephen didn't like too many similarities between
dp_aux_ep_client and dp_aux_ep_device. And I'd second him here.
>>> * We need to come up with a system for detecting when everything
>>> probes or is going to be removed, though that's probably not too hard.
>>> I guess the DP AUX bus could just replace the panel's probe function
>>> with its own and essentially "tail patch" it. I guess it could "head
>>> patch" the remove call? ...or is there some better way you were
>>> thinking of knowing when all our children probed?
>>>
>>> * The callback on the aux bus controller side would not be able to
>>> DEFER. In other words trying to acquire a reference to the panel can
>>> always be the last thing we do so we know there can be no reasons to
>>> defer after. This should be doable, but at least in the ps8640 case it
>>> will require changing the code a bit. I notice that today it actually
>>> tries to get the panel side _before_ it gets the MIPI side and it
>>> potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
>>> guess I have a niggling feeling that we'll find some reason in the
>>> future that we can't be last, but we can probably ignore that. ;-)
>>>
>>> I can switch this all to normal callbacks if that's what everyone
>>> wants, but it doesn't feel significantly cleaner to me and does seem
>>> to have some (small) downsides.
>>>
>>>
>>>> And this
>>>> approach would make driver's life easier, since e.g. the bus code can
>>>> pm_get the EP device before calling callbacks/notifiers and
>>>> pm_put_autosuspend it afterwards.
>>>
>>> Not sure about doing the pm calls on behalf of the EP device. What's
>>> the goal there?
>>
>> I think any driver can pm_runtime_get another device. The goal is to
>> let the 'post_probe' callback to power up the panel, read the EDID,
>> etc.
>
> Right. I was hoping to keep this as a separate discussion since I
> think it's largely unrelated to the probe ordering issue, but we can
> talk about it here if you want.
As for me they are pretty much tired one to another. As reading EDID
(even if it is just to read the panel ID) is one of the main issue with
panel probe path. I just don't want to end up in a situation when we
refactor aux_bus probe to fix the ordering/race issue and then we have
to refactor it again for reading EDID.
>
> There are a lot of open questions here and it's definitely hard to
> wrap your head around all of it. Maybe I'll just spam some thoughts
> and see if they all make sense together...
Thank you for the lengthy explanation. And I should be your pardon for
partially ignoring DP/ dp bridges patches earlier.
>
> 1. At the moment, there's no guarantee that a DP AUX Endpoint (AKA
> panel) will use pm_runtime() to power itself up enough to do an AUX
> transfer. At the moment the two eDP panels drivers I'm aware of use
> pm_runtime, but that's actually a fairly new behavior. I guess we'd
> have to codify it as "required" if we were going to rely on it.
* document it as a "required"
>
> 2. In general, panels have powered themselves enough to read the EDID
> in their prepare() stage, which is equivalent to the bridge's
> pre_enable(). During some of my early patches to try to support EDID
> reading in ti-sn65dsi86 I actually relied upon it. It was like that in
> v3 [1]. Personally I see this as the "official" interface to power on
> the panel from the DP controller. As such I'm not sure we need to add
> pm_runtime() as an equivalent option.
>
> 3. In the cover letter of v4 of my ti-sn65dsi86 EDID patch series I
> talked about why I switched to having EDID reading driven by the panel
> instead of powering on the panel (via pre_enable) and reading the EDID
> in the controller. One reason talked about there is that the "generic"
> eDP panel driver actually needs the EDID, or at least enough of it to
> get the panel ID, so that it can adjust its power sequence timings. If
> the EDID reading is completely handled by the DP driver and the panel
> can't do it then we'd need to figure out how to communicate it back.
I think with the current drm_bridge_connector-based code this should be
handled properly. Anyway, it should be the panel, who reads the EDID,
not the DP core. Actually just a random idea that just came to my mind.
Maybe (!) we should break ties between msm dp core and the whole
EDID/HPD/dp_panel story. In other words, split the whole DP EDID reading
to the separate drm_bridge. Maybe I'm overengineering it here.
>
> 4. In general, panels can be pretty persnickety about their power
> sequencing. As far as I've been able to tell, the official spec
> provides two things you can do:
>
> 4a) You can power the panel up enough to do AUX transfers and then
> power it back off.
>
> 4b) You can power the panel up enough to do AUX transfers, then finish
> powering it all the way up (turn on screen, backlight, etc). When you
> turn the screen off, if you follow the spec strictly, you're also
> _required_ to fully power the panel off. In other words, remove _all_
> power from the display including any power that would be needed to do
> AUX transfers.
Ugh. It's a pity that we can not leave AUX enabled forever, while doing
all kinds of turning the screen off and on again.
>
> Now the generic eDP panel code doesn't currently follow the
> "strict"ness of the spec and I'm not actually sure if that's how the
> spec is intended to be interpreted anyway. There are two timing
> diagrams, though. One for "aux transfer only" and the other for
> "normal system operation". In the "normal system operation" the
> diagram doesn't allow for the backlight to ever go off and on again.
>
> Now, despite the fact that the generic eDP panel code doesn't follow
> the "strict"ness I just described, the _other_ DP panel I worked on
> recently (samsung-atna33xc20) does. In testing we found that this
> panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> outputting data to the display and then started again. You absolutely
> needed to fully power cycle the display each time. I tried to document
> this to the best of my ability in atana33xc20_unprepare(). There's
> also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> is doing something the panel driver doesn't expect. I've also been
> trying to keep my eyes out to see if we need to do the same thing in
> generic eDP panel code, either for everyone or via some type of
> per-panel quirk. There's definitely a good reason to avoid the extra
> cycling if possible since powering panels off and on again often
> requires hundreds of milliseconds of delay in order to meet timing
> diagrams. ...and we do this if we ever change panel "modes".
Point noted.
>
> ...OK, so why does this all matter? I guess my point here is I worry a
> little bit about saying that the DP controller code can willy nilly
> request the panel to be powered whenever it wants. If the DP
> controller was trying to hold the panel powered and then we _needed_
> to power the panel off then that would be bad. It doesn't mean we
> can't be careful about it, of course...
>
> Said another way, in my mental model these three sequences are allowed:
>
> s1) prepare, unprepare
> s2) prepare, enable, disable, unprepare
> s3) prepare, enable, disable, unprepare, prepare, enable, disable, unprepare
>
> ...and this sequence is _not_ allowed:
>
> s4) prepare, enable, disable, enable, disable, unprepare
A strange random question (for which there is probably an existing
obvious answer somwewhere, 4 a.m. here).
Is there any reason why can't we drop prepare/unprepare for the eDP
panels and use the following sequence;
- get_modes() = perform AUX-only transfer the first time we hit the
function to read the EDID. return cached copy afterwards.
- a sequence of enable()/disable() calls doing a full powerup/powerdown?
>
> ...and, in my mind, it's up to the panel driver to know whether in
> sequence s3) it has to _force_ power off between the unprepare and a
> prepare.
>
> If pm_runtime() officially replaces prepare/unprepare then it's less
> obvious (in my mind) that we have to coordinate with enable().
I see
>
> 5. In general I've been asserting that it should be up to the panel to
> power things on and drive all AUX transactions. ...but clearly my
> model isn't reality. We certainly do AUX transactions from the DP
> driver because the DP driver needs to know things about the connected
> device, like the number of lanes it has, the version of eDP it
> supports, and the available bit rates to name a few. Those things all
> work today by relying on the fact that pre-enable powers the panel on.
> It's pretty easy to say that reading the EDID (and I guess AUX
> backlight) is the odd one out. So right now I guess my model is:
>
> 5a) If the panel code wants to access the AUX bus it can do so by
> powering itself on and then just doing an AUX transaction and assuming
> that the provider of the AUX bus can power itself on as needed.
>
> 5b) If the DP code wants to access the AUX bus it should make sure
> that the next bridge's pre_enable() has been called. It can then
> assume that the device is powered on until the next bridge's
> post_disable() has been called.
>
> So I guess tl;dr: I'm not really a huge fan of the DP driver powering
> the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
> with the interface that we have to pre_enable() the panel to turn it
> on.
Again, thank for the explanation. Your concerns make more sense now.
As much as I hate writing docs, maybe we should put at least basic notes
(regarding panel requirements) somewhere to the DP/DP AUX documentation?
>
>
> [1] https://lore.kernel.org/r/20210402152701.v3.8.Ied721dc895156046ac523baa55a71da241cd09c7@changeid/
> [2] https://lore.kernel.org/r/[email protected]/
>
>
>> BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
>> write the EDID checksum like we do for DP?
>
> Write the EDID checksum? I don't know what that means. You mean
> dp_panel_get_edid_checksum()? I'm not 100% sure, a quick glance seems
> to make me feel it has to do with DP compliance testing? I can dig
> more if need be. The generic EDID reading code already calculates the
> checksum, so unless you're doing some funny business you shouldn't
> need to check it again...
I was thinking about dp_link_send_edid_checksum() /
drm_dp_send_real_edid_checksum().
>
>
>> Do you have any good summary for eDP vs DP differences?
>
> I don't. :( Mostly stuff here is me trying to grok bits out of what
> existing drivers were doing and trying to cross reference it with the
> eDP spec that I have (which I don't believe I can share,
> unfortunately).
I'll check if I can get DP and eDP specs on my own.
--
With best wishes
Dmitry
Hi,
On Fri, Apr 15, 2022 at 5:54 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> >>> * They rely on there being exactly 1 AUX device, or we make it a rule
> >>> that we wait for all AUX devices to probe (?)
> >>
> >> Is the backlight a separate device on an AUX bus? Judging from
> >> drm_panel_dp_aux_backlight(), it isn't. I assumed that aux bus is just
> >> a point-to-point bus, so there is always a single client.
> >
> > Define "device". ;-)
>
> "a device on the AUX bus" = the device, which lists dp_aux_bus_type as
> dev->bus_type.
Right. So I guess I was thinking that we _could_ have modeled the
backlight as a device which lists dp_aux_bus_type as dev->bus_type.
AKA:
1. We could have had a second node in the sc7180-trogdor-homestar
device tree under the DP controller for the DP AUX backlight.
2. Instead of manually calling drm_panel_dp_aux_backlight() from
panel-edp.c and panel-samsung-atna33xc20.c the backlight could have
just probed on its own.
3. In the device tree, the panel could have had a link to the
backlight's phandle which would have made the existing
drm_panel_of_backlight() "just work" and we wouldn't have needed the
manual call to drm_panel_dp_aux_backlight().
Oh. But. Maybe. Not.
We couldn't really have done that because we would have been able to
do the DP AUX transactions for the backlight without powering on the
panel. So we couldn't really have probed them separately. OK, you guys
win this round. ;-)
> > It's a seperate "struct device" from a Linux point of view since it's
> > a backlight class device. Certainly it's highly correlated to the
> > display, but one can conceptually think of them as different devices,
> > sorta. ;-)
> >
> > I actually dug a tiny bit more into the whole "touchscreen over aux".
> > I guess DP 1.2 has a standard of "USB over DP AUX". No idea how that
> > would be modeled, of course.
>
> Ugh. Do you have any details of the standard itself? Like how does it
> looks like from the host point of view. And if the AUX is required to be
> powered for this USB bus to work?
>
> In other words: if we were to model it at this moment, would it be the
> child of the panel device (like backlight) or a separate device sitting
> on the same AUX bus?
I spent a bunch of time searching and I couldn't find more than a
reference, like "hey we came up with this idea and we're gonna write
down in the spec that you could totally do USB over the AUX channel,
but ummmm, we haven't actually implemented it yet". ;-) I certainly
could be wrong, but all of the references I could find were distinctly
lacking in details or pointers to other docs w/ more info.
...but while searching I _did_ find a lot of details (in the eDP 1.4
spec) about "Multi-touch over AUX". That looks like something that's
actually more well thought out and maybe even implemented somewhere.
From what I can tell though, it looks as if it's the same thing as the
backlight. In other words it's only available when the panel is
powered.
I don't think we want to do anything so drastic like moving the
ownership of panel power to the AUX bus or anything, so I guess this
means that:
a) The panel driver will remain in charge of powering the panel
(including anything on the DP AUX bus) on and off and getting the
power sequencing right.
b) That means that we can really think of the panel as the _only_
thing on the DP AUX bus.
c) Anything else on the DP AUX bus will be underneath the panel driver.
> > I guess the summary is that I'm OK w/ changing it to assume one device
> > for now, but I'm still not sure it's compelling to move to normal
> > callbacks. The API for callbacks is pretty much the same as the one I
> > proposed and IMO leaving it the way it is (with an extra struct
> > device) doesn't really add much complexity and has a few (small) nice
> > benefits.
>
> I think Stephen didn't like too many similarities between
> dp_aux_ep_client and dp_aux_ep_device. And I'd second him here.
>
>
> >>> * We need to come up with a system for detecting when everything
> >>> probes or is going to be removed, though that's probably not too hard.
> >>> I guess the DP AUX bus could just replace the panel's probe function
> >>> with its own and essentially "tail patch" it. I guess it could "head
> >>> patch" the remove call? ...or is there some better way you were
> >>> thinking of knowing when all our children probed?
> >>>
> >>> * The callback on the aux bus controller side would not be able to
> >>> DEFER. In other words trying to acquire a reference to the panel can
> >>> always be the last thing we do so we know there can be no reasons to
> >>> defer after. This should be doable, but at least in the ps8640 case it
> >>> will require changing the code a bit. I notice that today it actually
> >>> tries to get the panel side _before_ it gets the MIPI side and it
> >>> potentially can return -EPROBE_DEFER if it can't find the MIPI side. I
> >>> guess I have a niggling feeling that we'll find some reason in the
> >>> future that we can't be last, but we can probably ignore that. ;-)
> >>>
> >>> I can switch this all to normal callbacks if that's what everyone
> >>> wants, but it doesn't feel significantly cleaner to me and does seem
> >>> to have some (small) downsides.
> >>>
> >>>
> >>>> And this
> >>>> approach would make driver's life easier, since e.g. the bus code can
> >>>> pm_get the EP device before calling callbacks/notifiers and
> >>>> pm_put_autosuspend it afterwards.
> >>>
> >>> Not sure about doing the pm calls on behalf of the EP device. What's
> >>> the goal there?
> >>
> >> I think any driver can pm_runtime_get another device. The goal is to
> >> let the 'post_probe' callback to power up the panel, read the EDID,
> >> etc.
> >
> > Right. I was hoping to keep this as a separate discussion since I
> > think it's largely unrelated to the probe ordering issue, but we can
> > talk about it here if you want.
>
> As for me they are pretty much tired one to another. As reading EDID
> (even if it is just to read the panel ID) is one of the main issue with
> panel probe path. I just don't want to end up in a situation when we
> refactor aux_bus probe to fix the ordering/race issue and then we have
> to refactor it again for reading EDID.
>
> >
> > There are a lot of open questions here and it's definitely hard to
> > wrap your head around all of it. Maybe I'll just spam some thoughts
> > and see if they all make sense together...
>
> Thank you for the lengthy explanation. And I should be your pardon for
> partially ignoring DP/ dp bridges patches earlier.
>
> >
> > 1. At the moment, there's no guarantee that a DP AUX Endpoint (AKA
> > panel) will use pm_runtime() to power itself up enough to do an AUX
> > transfer. At the moment the two eDP panels drivers I'm aware of use
> > pm_runtime, but that's actually a fairly new behavior. I guess we'd
> > have to codify it as "required" if we were going to rely on it.
>
> * document it as a "required"
>
> >
> > 2. In general, panels have powered themselves enough to read the EDID
> > in their prepare() stage, which is equivalent to the bridge's
> > pre_enable(). During some of my early patches to try to support EDID
> > reading in ti-sn65dsi86 I actually relied upon it. It was like that in
> > v3 [1]. Personally I see this as the "official" interface to power on
> > the panel from the DP controller. As such I'm not sure we need to add
> > pm_runtime() as an equivalent option.
> >
> > 3. In the cover letter of v4 of my ti-sn65dsi86 EDID patch series I
> > talked about why I switched to having EDID reading driven by the panel
> > instead of powering on the panel (via pre_enable) and reading the EDID
> > in the controller. One reason talked about there is that the "generic"
> > eDP panel driver actually needs the EDID, or at least enough of it to
> > get the panel ID, so that it can adjust its power sequence timings. If
> > the EDID reading is completely handled by the DP driver and the panel
> > can't do it then we'd need to figure out how to communicate it back.
>
> I think with the current drm_bridge_connector-based code this should be
> handled properly. Anyway, it should be the panel, who reads the EDID,
> not the DP core. Actually just a random idea that just came to my mind.
> Maybe (!) we should break ties between msm dp core and the whole
> EDID/HPD/dp_panel story. In other words, split the whole DP EDID reading
> to the separate drm_bridge. Maybe I'm overengineering it here.
I had similar inklings before but I never explored it. I have a vague
recollection of mentioning it to Laurent and him thinking it was a bad
idea, though. No idea if I just dreamed that, though.
> > 4. In general, panels can be pretty persnickety about their power
> > sequencing. As far as I've been able to tell, the official spec
> > provides two things you can do:
> >
> > 4a) You can power the panel up enough to do AUX transfers and then
> > power it back off.
> >
> > 4b) You can power the panel up enough to do AUX transfers, then finish
> > powering it all the way up (turn on screen, backlight, etc). When you
> > turn the screen off, if you follow the spec strictly, you're also
> > _required_ to fully power the panel off. In other words, remove _all_
> > power from the display including any power that would be needed to do
> > AUX transfers.
>
> Ugh. It's a pity that we can not leave AUX enabled forever, while doing
> all kinds of turning the screen off and on again.
Yeah. I mean, it's possible that the issues with the Samsung panel
would have been solvable some other way, but definitely when we were
having problems with it the HW guys who were looking at it were
claiming that it was a violation and I couldn't find anything to prove
them wrong. :-( ...and fully power cycling fixed all the problems...
> > Now the generic eDP panel code doesn't currently follow the
> > "strict"ness of the spec and I'm not actually sure if that's how the
> > spec is intended to be interpreted anyway. There are two timing
> > diagrams, though. One for "aux transfer only" and the other for
> > "normal system operation". In the "normal system operation" the
> > diagram doesn't allow for the backlight to ever go off and on again.
> >
> > Now, despite the fact that the generic eDP panel code doesn't follow
> > the "strict"ness I just described, the _other_ DP panel I worked on
> > recently (samsung-atna33xc20) does. In testing we found that this
> > panel would sometimes (like 1 in 20 times?) crash if you ever stopped
> > outputting data to the display and then started again. You absolutely
> > needed to fully power cycle the display each time. I tried to document
> > this to the best of my ability in atana33xc20_unprepare(). There's
> > also a WARN_ON() in atana33xc20_enable() trying to detect if someone
> > is doing something the panel driver doesn't expect. I've also been
> > trying to keep my eyes out to see if we need to do the same thing in
> > generic eDP panel code, either for everyone or via some type of
> > per-panel quirk. There's definitely a good reason to avoid the extra
> > cycling if possible since powering panels off and on again often
> > requires hundreds of milliseconds of delay in order to meet timing
> > diagrams. ...and we do this if we ever change panel "modes".
>
> Point noted.
>
> >
> > ...OK, so why does this all matter? I guess my point here is I worry a
> > little bit about saying that the DP controller code can willy nilly
> > request the panel to be powered whenever it wants. If the DP
> > controller was trying to hold the panel powered and then we _needed_
> > to power the panel off then that would be bad. It doesn't mean we
> > can't be careful about it, of course...
> >
> > Said another way, in my mental model these three sequences are allowed:
> >
> > s1) prepare, unprepare
> > s2) prepare, enable, disable, unprepare
> > s3) prepare, enable, disable, unprepare, prepare, enable, disable, unprepare
> >
> > ...and this sequence is _not_ allowed:
> >
> > s4) prepare, enable, disable, enable, disable, unprepare
>
> A strange random question (for which there is probably an existing
> obvious answer somwewhere, 4 a.m. here).
>
> Is there any reason why can't we drop prepare/unprepare for the eDP
> panels and use the following sequence;
>
> - get_modes() = perform AUX-only transfer the first time we hit the
> function to read the EDID. return cached copy afterwards.
side note: we already do cache the EDID.
> - a sequence of enable()/disable() calls doing a full powerup/powerdown?
Aside from the fact that there's a bunch of code out there that
assumes that panels are powered on in pre_enable() ? For instance, on
parade-ps8640 we still support old device trees where the panel isn't
listed under the DP AUX bus. These devices only support a hardcoded
panel, not the generic "edp-panel". There you can see that
ps8640_bridge_get_edid() assumes it can power the panel on with
drm_bridge_chain_pre_enable()
...but besides that, it still wouldn't work. For reasons I won't get
into, here's the current order the bridge chains are called for
prepare and enable:
pre_enable: start from connector and move to encoder
enable: start from encoder and move to connector
So if we stop turning on panel power in pre_enable() then when the DP
controller's enable() is running the panel won't have any power. I
don't think that will be so bueno.
> > ...and, in my mind, it's up to the panel driver to know whether in
> > sequence s3) it has to _force_ power off between the unprepare and a
> > prepare.
> >
> > If pm_runtime() officially replaces prepare/unprepare then it's less
> > obvious (in my mind) that we have to coordinate with enable().
>
> I see
>
> >
> > 5. In general I've been asserting that it should be up to the panel to
> > power things on and drive all AUX transactions. ...but clearly my
> > model isn't reality. We certainly do AUX transactions from the DP
> > driver because the DP driver needs to know things about the connected
> > device, like the number of lanes it has, the version of eDP it
> > supports, and the available bit rates to name a few. Those things all
> > work today by relying on the fact that pre-enable powers the panel on.
> > It's pretty easy to say that reading the EDID (and I guess AUX
> > backlight) is the odd one out. So right now I guess my model is:
> >
> > 5a) If the panel code wants to access the AUX bus it can do so by
> > powering itself on and then just doing an AUX transaction and assuming
> > that the provider of the AUX bus can power itself on as needed.
> >
> > 5b) If the DP code wants to access the AUX bus it should make sure
> > that the next bridge's pre_enable() has been called. It can then
> > assume that the device is powered on until the next bridge's
> > post_disable() has been called.
> >
> > So I guess tl;dr: I'm not really a huge fan of the DP driver powering
> > the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
> > with the interface that we have to pre_enable() the panel to turn it
> > on.
>
> Again, thank for the explanation. Your concerns make more sense now.
> As much as I hate writing docs, maybe we should put at least basic notes
> (regarding panel requirements) somewhere to the DP/DP AUX documentation?
Sure. I actually don't mind writing docs, but my problem is trying to
figure out where the heck to put them where someone would actually
notice them. I could throw a blurb in the kernel doc for `struct
drm_dp_aux` I guess? How about a deal: if you can tell me where to put
the above facts (essentially 5a and 5b) then I'm happy to post a patch
adding them.
Huh, actually, maybe the right place is in the "transfer" function of
that structure which, as of commit bacbab58f09d ("drm: Mention the
power state requirement on side-channel operations"), actually has a
blurb. ...but I don't think the blurb there is totally complete. What
if I changed it to this:
* Also note that this callback can be called no matter the
* state @dev is in and also no matter what state the panel is
* in. It's expected:
* - If the @dev providing the AUX bus is currently unpowered then
* it will power itself up for the transfer.
* - If the panel is not in a state where it can respond (it's not
* powered or it's in a low power state) then this function will
* fail. Note that if a panel driver is initiating a DP AUX transfer
* it may power itself up however it wants. All other code should
* use the pre_enable() bridge chain (which eventually calls the
* panel prepare function) to power the panel.
> > [1] https://lore.kernel.org/r/20210402152701.v3.8.Ied721dc895156046ac523baa55a71da241cd09c7@changeid/
> > [2] https://lore.kernel.org/r/[email protected]/
> >
> >
> >> BTW: as I'm slowly diving into DP vs eDP differences. Do we need to
> >> write the EDID checksum like we do for DP?
> >
> > Write the EDID checksum? I don't know what that means. You mean
> > dp_panel_get_edid_checksum()? I'm not 100% sure, a quick glance seems
> > to make me feel it has to do with DP compliance testing? I can dig
> > more if need be. The generic EDID reading code already calculates the
> > checksum, so unless you're doing some funny business you shouldn't
> > need to check it again...
>
> I was thinking about dp_link_send_edid_checksum() /
That one seems to be only used for DP_TEST_LINK_EDID_READ. I _think_
that's for DP compliance testing and I don't think we need compliance
testing for eDP.
> drm_dp_send_real_edid_checksum().
I don't see calls to this right now for Qualcomm, but it also looks
related to compliance testing?
So I guess where does that leave us? Maybe:
1. I'll add a WARN_ON() in of_dp_aux_populate_ep_devices() if there is
more than one DP AUX endpoint with a comment explaining why we assume
one DP AUX endpoint.
2. I'll create this new structure in drm_dp_aux_bus.h:
struct dp_aux_populate_callbacks {
int (*done_probing)(struct drm_dp_aux *aux);
void (*pre_remove)(struct drm_dp_aux *aux);
};
3. I'll add a second version of the populate functions that AUX bus
providers can use if they want callbacks:
int of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
struct dp_aux_populate_callbacks *cb);
int devm_of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
struct dp_aux_populate_callbacks *cb);
The old functions will just be changed to wrap the above and pass NULL
for the callbacks. To me, this seems better/simpler than notifiers or
any other scheme, but yell if you disagree.
4. I'll call the callsbacks in dp_aux_ep_probe() after a successful
probe. I'll add a second callback and will call it in
dp_aux_ep_remove() before passing the remove through to the panel.
If that sounds peachy then I think it should be pretty doable.
-Doug
Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson <[email protected]> wrote:
>
> > > 5. In general I've been asserting that it should be up to the panel to
> > > power things on and drive all AUX transactions. ...but clearly my
> > > model isn't reality. We certainly do AUX transactions from the DP
> > > driver because the DP driver needs to know things about the connected
> > > device, like the number of lanes it has, the version of eDP it
> > > supports, and the available bit rates to name a few. Those things all
> > > work today by relying on the fact that pre-enable powers the panel on.
> > > It's pretty easy to say that reading the EDID (and I guess AUX
> > > backlight) is the odd one out. So right now I guess my model is:
> > >
> > > 5a) If the panel code wants to access the AUX bus it can do so by
> > > powering itself on and then just doing an AUX transaction and assuming
> > > that the provider of the AUX bus can power itself on as needed.
> > >
> > > 5b) If the DP code wants to access the AUX bus it should make sure
> > > that the next bridge's pre_enable() has been called. It can then
> > > assume that the device is powered on until the next bridge's
> > > post_disable() has been called.
> > >
> > > So I guess tl;dr: I'm not really a huge fan of the DP driver powering
> > > the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
> > > with the interface that we have to pre_enable() the panel to turn it
> > > on.
> >
> > Again, thank for the explanation. Your concerns make more sense now.
> > As much as I hate writing docs, maybe we should put at least basic notes
> > (regarding panel requirements) somewhere to the DP/DP AUX documentation?
>
> Sure. I actually don't mind writing docs, but my problem is trying to
> figure out where the heck to put them where someone would actually
> notice them. I could throw a blurb in the kernel doc for `struct
> drm_dp_aux` I guess? How about a deal: if you can tell me where to put
> the above facts (essentially 5a and 5b) then I'm happy to post a patch
> adding them.
>
> Huh, actually, maybe the right place is in the "transfer" function of
> that structure which, as of commit bacbab58f09d ("drm: Mention the
> power state requirement on side-channel operations"), actually has a
> blurb. ...but I don't think the blurb there is totally complete. What
> if I changed it to this:
>
> * Also note that this callback can be called no matter the
> * state @dev is in and also no matter what state the panel is
> * in. It's expected:
> * - If the @dev providing the AUX bus is currently unpowered then
> * it will power itself up for the transfer.
> * - If the panel is not in a state where it can respond (it's not
> * powered or it's in a low power state) then this function will
> * fail. Note that if a panel driver is initiating a DP AUX transfer
> * it may power itself up however it wants. All other code should
> * use the pre_enable() bridge chain (which eventually calls the
> * panel prepare function) to power the panel.
I didn't ignore this documentation request. Please take a look here
and see what you think:
https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid
-Doug
Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson <[email protected]> wrote:
>
> So I guess where does that leave us? Maybe:
>
> 1. I'll add a WARN_ON() in of_dp_aux_populate_ep_devices() if there is
> more than one DP AUX endpoint with a comment explaining why we assume
> one DP AUX endpoint.
>
> 2. I'll create this new structure in drm_dp_aux_bus.h:
>
> struct dp_aux_populate_callbacks {
> int (*done_probing)(struct drm_dp_aux *aux);
> void (*pre_remove)(struct drm_dp_aux *aux);
> };
>
> 3. I'll add a second version of the populate functions that AUX bus
> providers can use if they want callbacks:
>
> int of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
> struct dp_aux_populate_callbacks *cb);
> int devm_of_dp_aux_populate_ep_devices_cb(struct drm_dp_aux *aux,
> struct dp_aux_populate_callbacks *cb);
>
> The old functions will just be changed to wrap the above and pass NULL
> for the callbacks. To me, this seems better/simpler than notifiers or
> any other scheme, but yell if you disagree.
>
> 4. I'll call the callsbacks in dp_aux_ep_probe() after a successful
> probe. I'll add a second callback and will call it in
> dp_aux_ep_remove() before passing the remove through to the panel.
>
>
> If that sounds peachy then I think it should be pretty doable.
I never heard any response about whether people liked the above, but I
went ahead and did something similar to it. It can be found at:
https://lore.kernel.org/r/[email protected]