[2/2] drm/bridge: ps8640: Add a cache for EDID
Commit Message
When there are multiple EDID reads, the bridge will be repeatedly
enabled and disabled. Add a cache for EDID to speed this up.
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---
drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
1 file changed, 37 insertions(+), 24 deletions(-)
Comments
On Tue, Mar 14, 2023 at 12:01 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> When there are multiple EDID reads, the bridge will be repeatedly
> enabled and disabled. Add a cache for EDID to speed this up.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 08de501c436e..4d594be8b89c 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -105,6 +105,7 @@ struct ps8640 {
> struct gpio_desc *gpio_reset;
> struct gpio_desc *gpio_powerdown;
> struct device_link *link;
> + struct edid *edid;
> bool pre_enabled;
> bool need_post_hpd_delay;
> };
> @@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> {
> struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> bool poweroff = !ps_bridge->pre_enabled;
> - struct edid *edid;
>
> - /*
> - * When we end calling get_edid() triggered by an ioctl, i.e
> - *
> - * drm_mode_getconnector (ioctl)
> - * -> drm_helper_probe_single_connector_modes
> - * -> drm_bridge_connector_get_modes
> - * -> ps8640_bridge_get_edid
> - *
> - * We need to make sure that what we need is enabled before reading
> - * EDID, for this chip, we need to do a full poweron, otherwise it will
> - * fail.
> - */
> - if (poweroff)
> - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> + if (!ps_bridge->edid) {
> + /*
> + * When we end calling get_edid() triggered by an ioctl, i.e
> + *
> + * drm_mode_getconnector (ioctl)
> + * -> drm_helper_probe_single_connector_modes
> + * -> drm_bridge_connector_get_modes
> + * -> ps8640_bridge_get_edid
> + *
> + * We need to make sure that what we need is enabled before
> + * reading EDID, for this chip, we need to do a full poweron,
> + * otherwise it will fail.
> + */
> + if (poweroff)
> + drm_atomic_bridge_chain_pre_enable(bridge,
> + connector->state->state);
>
> - edid = drm_get_edid(connector,
> - ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> + ps_bridge->edid = drm_get_edid(connector,
> + ps_bridge->page[PAGE0_DP_CNTL]->adapter);
>
> - /*
> - * If we call the get_edid() function without having enabled the chip
> - * before, return the chip to its original power state.
> - */
> - if (poweroff)
> - drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
> + /*
> + * If we call the get_edid() function without having enabled the
> + * chip before, return the chip to its original power state.
> + */
> + if (poweroff)
> + drm_atomic_bridge_chain_post_disable(bridge,
> + connector->state->state);
> + }
>
> - return edid;
> + return drm_edid_duplicate(ps_bridge->edid);
> }
>
> static void ps8640_runtime_disable(void *data)
> @@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
> return ret;
> }
>
> +static void ps8640_remove(struct i2c_client *client)
> +{
> + struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> +
> + kfree(ps_bridge->edid);
> + ps_bridge->edid = NULL;
> +}
> +
> static const struct of_device_id ps8640_match[] = {
> { .compatible = "parade,ps8640" },
> { }
> @@ -775,6 +787,7 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
>
> static struct i2c_driver ps8640_driver = {
> .probe_new = ps8640_probe,
> + .remove = ps8640_remove,
> .driver = {
> .name = "ps8640",
> .of_match_table = ps8640_match,
> --
> 2.40.0.rc1.284.g88254d51c5-goog
>
Reviewed-by: Robert Foss <rfoss@kernel.org>
Hi,
On Tue, Mar 14, 2023 at 4:00 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> When there are multiple EDID reads, the bridge will be repeatedly
> enabled and disabled. Add a cache for EDID to speed this up.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 61 ++++++++++++++++----------
> 1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 08de501c436e..4d594be8b89c 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -105,6 +105,7 @@ struct ps8640 {
> struct gpio_desc *gpio_reset;
> struct gpio_desc *gpio_powerdown;
> struct device_link *link;
> + struct edid *edid;
> bool pre_enabled;
> bool need_post_hpd_delay;
> };
> @@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> {
> struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> bool poweroff = !ps_bridge->pre_enabled;
> - struct edid *edid;
>
> - /*
> - * When we end calling get_edid() triggered by an ioctl, i.e
> - *
> - * drm_mode_getconnector (ioctl)
> - * -> drm_helper_probe_single_connector_modes
> - * -> drm_bridge_connector_get_modes
> - * -> ps8640_bridge_get_edid
> - *
> - * We need to make sure that what we need is enabled before reading
> - * EDID, for this chip, we need to do a full poweron, otherwise it will
> - * fail.
> - */
> - if (poweroff)
> - drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
> + if (!ps_bridge->edid) {
> + /*
> + * When we end calling get_edid() triggered by an ioctl, i.e
> + *
> + * drm_mode_getconnector (ioctl)
> + * -> drm_helper_probe_single_connector_modes
> + * -> drm_bridge_connector_get_modes
> + * -> ps8640_bridge_get_edid
> + *
> + * We need to make sure that what we need is enabled before
> + * reading EDID, for this chip, we need to do a full poweron,
> + * otherwise it will fail.
> + */
> + if (poweroff)
> + drm_atomic_bridge_chain_pre_enable(bridge,
> + connector->state->state);
>
> - edid = drm_get_edid(connector,
> - ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> + ps_bridge->edid = drm_get_edid(connector,
> + ps_bridge->page[PAGE0_DP_CNTL]->adapter);
>
> - /*
> - * If we call the get_edid() function without having enabled the chip
> - * before, return the chip to its original power state.
> - */
> - if (poweroff)
> - drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
> + /*
> + * If we call the get_edid() function without having enabled the
> + * chip before, return the chip to its original power state.
> + */
> + if (poweroff)
> + drm_atomic_bridge_chain_post_disable(bridge,
> + connector->state->state);
> + }
>
> - return edid;
> + return drm_edid_duplicate(ps_bridge->edid);
> }
>
> static void ps8640_runtime_disable(void *data)
> @@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
> return ret;
> }
>
> +static void ps8640_remove(struct i2c_client *client)
> +{
> + struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> +
> + kfree(ps_bridge->edid);
> + ps_bridge->edid = NULL;
nit: no need to clear this to NULL since the driver is exiting.
Other than the small nit:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
@@ -105,6 +105,7 @@ struct ps8640 {
struct gpio_desc *gpio_reset;
struct gpio_desc *gpio_powerdown;
struct device_link *link;
+ struct edid *edid;
bool pre_enabled;
bool need_post_hpd_delay;
};
@@ -543,34 +544,37 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
{
struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
bool poweroff = !ps_bridge->pre_enabled;
- struct edid *edid;
- /*
- * When we end calling get_edid() triggered by an ioctl, i.e
- *
- * drm_mode_getconnector (ioctl)
- * -> drm_helper_probe_single_connector_modes
- * -> drm_bridge_connector_get_modes
- * -> ps8640_bridge_get_edid
- *
- * We need to make sure that what we need is enabled before reading
- * EDID, for this chip, we need to do a full poweron, otherwise it will
- * fail.
- */
- if (poweroff)
- drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
+ if (!ps_bridge->edid) {
+ /*
+ * When we end calling get_edid() triggered by an ioctl, i.e
+ *
+ * drm_mode_getconnector (ioctl)
+ * -> drm_helper_probe_single_connector_modes
+ * -> drm_bridge_connector_get_modes
+ * -> ps8640_bridge_get_edid
+ *
+ * We need to make sure that what we need is enabled before
+ * reading EDID, for this chip, we need to do a full poweron,
+ * otherwise it will fail.
+ */
+ if (poweroff)
+ drm_atomic_bridge_chain_pre_enable(bridge,
+ connector->state->state);
- edid = drm_get_edid(connector,
- ps_bridge->page[PAGE0_DP_CNTL]->adapter);
+ ps_bridge->edid = drm_get_edid(connector,
+ ps_bridge->page[PAGE0_DP_CNTL]->adapter);
- /*
- * If we call the get_edid() function without having enabled the chip
- * before, return the chip to its original power state.
- */
- if (poweroff)
- drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
+ /*
+ * If we call the get_edid() function without having enabled the
+ * chip before, return the chip to its original power state.
+ */
+ if (poweroff)
+ drm_atomic_bridge_chain_post_disable(bridge,
+ connector->state->state);
+ }
- return edid;
+ return drm_edid_duplicate(ps_bridge->edid);
}
static void ps8640_runtime_disable(void *data)
@@ -767,6 +771,14 @@ static int ps8640_probe(struct i2c_client *client)
return ret;
}
+static void ps8640_remove(struct i2c_client *client)
+{
+ struct ps8640 *ps_bridge = i2c_get_clientdata(client);
+
+ kfree(ps_bridge->edid);
+ ps_bridge->edid = NULL;
+}
+
static const struct of_device_id ps8640_match[] = {
{ .compatible = "parade,ps8640" },
{ }
@@ -775,6 +787,7 @@ 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,