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/1648656179-10347-2-git-send-email-quic_sbillaka@qu...
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(-)
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 dianders@chromium.org ---
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_ */
On Fri, 08 Apr 2022, Douglas Anderson dianders@chromium.org 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 dianders@chromium.org
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_ */
Hi,
On Mon, Apr 11, 2022 at 1:34 AM Jani Nikula jani.nikula@linux.intel.com wrote:
On Fri, 08 Apr 2022, Douglas Anderson dianders@chromium.org 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 dianders@chromium.org
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
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 dianders@chromium.org
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 4:52 PM Stephen Boyd swboyd@chromium.org 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 dianders@chromium.org
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
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 dianders@chromium.org
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_ */
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org 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?
On Sat, 16 Apr 2022 at 00:13, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org 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?
Hi,
On Fri, Apr 15, 2022 at 3:45 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On Sat, 16 Apr 2022 at 00:13, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org 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.Ied721dc895156046ac523baa55a71... [2] https://lore.kernel.org/r/20210416223950.3586967-1-dianders@chromium.org/
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 dmitry.baryshkov@linaro.org wrote:
On Sat, 16 Apr 2022 at 00:13, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:47 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org 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.
- 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"
- 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.
- 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.
- 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
- 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.Ied721dc895156046ac523baa55a71... [2] https://lore.kernel.org/r/20210416223950.3586967-1-dianders@chromium.org/
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.
Hi,
On Fri, Apr 15, 2022 at 5:54 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org 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.
- 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"
- 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.
- 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.
- 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
- 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.Ied721dc895156046ac523baa55a71... [2] https://lore.kernel.org/r/20210416223950.3586967-1-dianders@chromium.org/
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 dianders@chromium.org wrote:
So I guess where does that leave us? Maybe:
- 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.
- 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); };
- 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.
- 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/20220503224029.3195306-1-dianders@chromium.org
Hi,
On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson dianders@chromium.org wrote:
- 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.Ia8651894026707e4fa61267da944ff73...
-Doug
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 dianders@chromium.org ---
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,
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 dmitry.baryshkov@linaro.org Signed-off-by: Douglas Anderson dianders@chromium.org ---
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. */
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 dmitry.baryshkov@linaro.org Signed-off-by: Douglas Anderson dianders@chromium.org
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
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.
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org ---
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{ + return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted)); +} + +static bool panel_edp_read_hpd(struct panel_edp *p) +{ + if (p->hpd_gpio) + return gpiod_get_value_cansleep(p->hpd_gpio); + + return p->aux->is_hpd_asserted(p->aux); +} + static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev; @@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
- if (p->hpd_gpio) { + if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
- err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio, + /* + * Extra max delay, mostly to account for ps8640. 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. + */ + hpd_wait_us += 3000000; + + err = readx_poll_timeout(panel_edp_read_hpd, p, hpd_asserted, hpd_asserted, 1000, hpd_wait_us); if (hpd_asserted < 0) @@ -532,18 +553,22 @@ static int panel_edp_enable(struct drm_panel *panel) /* * If there is a "prepare_to_enable" delay then that's supposed to be * the delay from HPD going high until we can turn the backlight on. - * However, we can only count this if HPD is handled by the panel - * driver, not if it goes to a dedicated pin on the controller. + * However, we can only count this if HPD is readable by the panel + * driver. + * * If we aren't handling the HPD pin ourselves then the best we * can do is assume that HPD went high immediately before we were - * called (and link training took zero time). + * called (and link training took zero time). Note that "no-hpd" + * actually counts as handling HPD ourselves since we're doing the + * worst case delay (in prepare) ourselves. * * NOTE: if we ever end up in this "if" statement then we're * guaranteed that the panel_edp_wait() call below will do no delay. * It already handles that case, though, so we don't need any special * code for it. */ - if (p->desc->delay.prepare_to_enable && !p->hpd_gpio && !p->no_hpd) + if (p->desc->delay.prepare_to_enable && + !panel_edp_can_read_hpd(p) && !p->no_hpd) delay = max(delay, p->desc->delay.prepare_to_enable);
if (delay)
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
- return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));
+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
- if (p->hpd_gpio)
return gpiod_get_value_cansleep(p->hpd_gpio);- return p->aux->is_hpd_asserted(p->aux);
+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
- if (p->hpd_gpio) {
- if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;
I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
err = readx_poll_timeout(panel_edp_read_hpd, p, hpd_asserted, hpd_asserted, 1000, hpd_wait_us);@@ -532,18 +553,22 @@ static int panel_edp_enable(struct drm_panel *panel) /* * If there is a "prepare_to_enable" delay then that's supposed to be * the delay from HPD going high until we can turn the backlight on.
* However, we can only count this if HPD is handled by the panel* driver, not if it goes to a dedicated pin on the controller.
* However, we can only count this if HPD is readable by the panel* driver.*
- If we aren't handling the HPD pin ourselves then the best we
- can do is assume that HPD went high immediately before we were
* called (and link training took zero time).
* called (and link training took zero time). Note that "no-hpd"* actually counts as handling HPD ourselves since we're doing the* worst case delay (in prepare) ourselves.*/
- NOTE: if we ever end up in this "if" statement then we're
- guaranteed that the panel_edp_wait() call below will do no delay.
- It already handles that case, though, so we don't need any special
- code for it.
- if (p->desc->delay.prepare_to_enable && !p->hpd_gpio && !p->no_hpd)
if (p->desc->delay.prepare_to_enable &&
!panel_edp_can_read_hpd(p) && !p->no_hpd)delay = max(delay, p->desc->delay.prepare_to_enable);
if (delay)
Hi,
On Thu, Apr 14, 2022 at 5:51 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
if (p->hpd_gpio)return gpiod_get_value_cansleep(p->hpd_gpio);return p->aux->is_hpd_asserted(p->aux);+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
if (p->hpd_gpio) {
if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
What would you think about changing the API slightly? Instead of is_hpd_asserted(), we change it to wait_hpd_asserted() and it takes a timeout in microseconds. If you pass 0 for the timeout the function is defined to behave the same as is_hpd_asserted() today--AKA a single poll of the line.
-Doug
On Sat, 16 Apr 2022 at 00:17, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:51 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
if (p->hpd_gpio)return gpiod_get_value_cansleep(p->hpd_gpio);return p->aux->is_hpd_asserted(p->aux);+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
if (p->hpd_gpio) {
if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
What would you think about changing the API slightly? Instead of is_hpd_asserted(), we change it to wait_hpd_asserted() and it takes a timeout in microseconds. If you pass 0 for the timeout the function is defined to behave the same as is_hpd_asserted() today--AKA a single poll of the line.
This might work. Can you check it, please?
BTW: are these changes dependent on the first part of the patchset? It might be worth splitting the patchset into two parts.
Hi,
On Fri, Apr 15, 2022 at 3:12 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On Sat, 16 Apr 2022 at 00:17, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:51 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
if (p->hpd_gpio)return gpiod_get_value_cansleep(p->hpd_gpio);return p->aux->is_hpd_asserted(p->aux);+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
if (p->hpd_gpio) {
if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
What would you think about changing the API slightly? Instead of is_hpd_asserted(), we change it to wait_hpd_asserted() and it takes a timeout in microseconds. If you pass 0 for the timeout the function is defined to behave the same as is_hpd_asserted() today--AKA a single poll of the line.
This might work. Can you check it, please?
Cool. I'll spin with this. Hopefully early next week unless my inbox blows up. ...or my main PC's SSD like happened this week. ;-)
BTW: are these changes dependent on the first part of the patchset? It might be worth splitting the patchset into two parts.
Definitely not. As per the cover letter, this is two series jammed into one. I'm happy to split them up. The 2nd half seems much less controversial.
-Doug
On 16/04/2022 03:12, Doug Anderson wrote:
Hi,
On Fri, Apr 15, 2022 at 3:12 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On Sat, 16 Apr 2022 at 00:17, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:51 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
if (p->hpd_gpio)return gpiod_get_value_cansleep(p->hpd_gpio);return p->aux->is_hpd_asserted(p->aux);+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
if (p->hpd_gpio) {
if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
What would you think about changing the API slightly? Instead of is_hpd_asserted(), we change it to wait_hpd_asserted() and it takes a timeout in microseconds. If you pass 0 for the timeout the function is defined to behave the same as is_hpd_asserted() today--AKA a single poll of the line.
This might work. Can you check it, please?
Cool. I'll spin with this. Hopefully early next week unless my inbox blows up. ...or my main PC's SSD like happened this week. ;-)
BTW: are these changes dependent on the first part of the patchset? It might be worth splitting the patchset into two parts.
Definitely not. As per the cover letter, this is two series jammed into one. I'm happy to split them up. The 2nd half seems much less controversial.
Great, let's get it in then. As you have time.
Hi,
On Fri, Apr 15, 2022 at 5:14 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 16/04/2022 03:12, Doug Anderson wrote:
Hi,
On Fri, Apr 15, 2022 at 3:12 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On Sat, 16 Apr 2022 at 00:17, Doug Anderson dianders@chromium.org wrote:
Hi,
On Thu, Apr 14, 2022 at 5:51 PM Dmitry Baryshkov dmitry.baryshkov@linaro.org wrote:
On 09/04/2022 05:36, Douglas Anderson wrote:
Let's add support for being able to read the HPD pin even if it's hooked directly to the controller. This will allow us to get more accurate delays also lets us take away the waiting in the AUX transfer functions of the eDP controller drivers.
Signed-off-by: Douglas Anderson dianders@chromium.org
drivers/gpu/drm/panel/panel-edp.c | 37 ++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index 1732b4f56e38..4a143eb9544b 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -417,6 +417,19 @@ static int panel_edp_get_hpd_gpio(struct device *dev, struct panel_edp *p) return 0; }
+static bool panel_edp_can_read_hpd(struct panel_edp *p) +{
return !p->no_hpd && (p->hpd_gpio || (p->aux && p->aux->is_hpd_asserted));+}
+static bool panel_edp_read_hpd(struct panel_edp *p) +{
if (p->hpd_gpio)return gpiod_get_value_cansleep(p->hpd_gpio);return p->aux->is_hpd_asserted(p->aux);+}
- static int panel_edp_prepare_once(struct panel_edp *p) { struct device *dev = p->base.dev;
@@ -441,13 +454,21 @@ static int panel_edp_prepare_once(struct panel_edp *p) if (delay) msleep(delay);
if (p->hpd_gpio) {
if (panel_edp_can_read_hpd(p)) { if (p->desc->delay.hpd_absent) hpd_wait_us = p->desc->delay.hpd_absent * 1000UL; else hpd_wait_us = 2000000;
err = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
/** Extra max delay, mostly to account for ps8640. 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.*/hpd_wait_us += 3000000;I think this should come in a separate commit and ideally this should be configurable somehow. Other hosts wouldn't need such 'additional' delay.
With this change removed:
Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
What would you think about changing the API slightly? Instead of is_hpd_asserted(), we change it to wait_hpd_asserted() and it takes a timeout in microseconds. If you pass 0 for the timeout the function is defined to behave the same as is_hpd_asserted() today--AKA a single poll of the line.
This might work. Can you check it, please?
Cool. I'll spin with this. Hopefully early next week unless my inbox blows up. ...or my main PC's SSD like happened this week. ;-)
BTW: are these changes dependent on the first part of the patchset? It might be worth splitting the patchset into two parts.
Definitely not. As per the cover letter, this is two series jammed into one. I'm happy to split them up. The 2nd half seems much less controversial.
Great, let's get it in then. As you have time.
Breadcrumbs: I've posted this as:
https://lore.kernel.org/r/20220418171757.2282651-1-dianders@chromium.org
-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 dianders@chromium.org ---
.../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),
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 dianders@chromium.org ---
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);
dri-devel@lists.freedesktop.org
-
Dmitry Baryshkov -
Doug Anderson -
Douglas Anderson
-
Jani Nikula -
Stephen Boyd