[PATCH 0/3] drm/bridge: ps8640: Make sure all needed is powered to get the EDID

[PATCH 0/3] drm/bridge: ps8640: Make sure all needed is powered to get the EDID

[Enric Balletbo i Serra]

The first patch is not really related and can be picked alone. The
second and the third are to rework the power handling to get the EDID.
Basically, we need to make sure all the needed is powered to be able
to get the EDID. Before, we saw that getting the EDID failed as
explained in the third patch.

Note that this series depends on:

  https://lore.kernel.org/patchwork/project/lkml/list/?series=448525

to apply cleanly.


Enric Balletbo i Serra (3):
  drm/bridge: ps8640: Return an error for incorrect attach flags
  drm/bridge: ps8640: Print an error if VDO control fails
  drm/bridge: ps8640: Rework power state handling

 drivers/gpu/drm/bridge/parade-ps8640.c | 94 ++++++++++++++++++++++----
 1 file changed, 82 insertions(+), 12 deletions(-)

-- 
2.27.0

[PATCH 1/3] drm/bridge: ps8640: Return an error for incorrect attach flags

[Enric Balletbo i Serra]

Bridge drivers that implement the new model only shall return an error
from their attach() handler when the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag
is not set. So make sure we return an error because only the new
drm_bridge model is supported.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 13755d278db6d..ce3e8b2da8c9b 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -200,6 +200,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 						   .channel = 0,
 						   .node = NULL,
 						 };
+
+	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
+		return -EINVAL;
+
 	/* port@0 is ps8640 dsi input port */
 	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
 	if (!in_ep)
-- 
2.27.0

Re: [PATCH 1/3] drm/bridge: ps8640: Return an error for incorrect attach flags

[Sam Ravnborg]

Hi Enric.

On Mon, Jun 15, 2020 at 10:53:18PM +0200, Enric Balletbo i Serra wrote:
> Bridge drivers that implement the new model only shall return an error
> from their attach() handler when the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag
> is not set. So make sure we return an error because only the new
> drm_bridge model is supported.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

I will await the updated series before applying.
> ---
> 
>  drivers/gpu/drm/bridge/parade-ps8640.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 13755d278db6d..ce3e8b2da8c9b 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -200,6 +200,10 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>  						   .channel = 0,
>  						   .node = NULL,
>  						 };
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> +		return -EINVAL;
> +
>  	/* port@0 is ps8640 dsi input port */
>  	in_ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
>  	if (!in_ep)
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/bridge: ps8640: Print an error if VDO control fails

[Enric Balletbo i Serra]

Print an error message inside ps8640_bridge_vdo_control() function when
it fails so we can simplify a bit the callers, they will only need to
check the error code.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index ce3e8b2da8c9b..9f7b7a9c53c52 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -82,8 +82,11 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 	ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
 					     sizeof(vdo_ctrl_buf),
 					     vdo_ctrl_buf);
-	if (ret < 0)
+	if (ret < 0) {
+		DRM_ERROR("failed to %sable VDO: %d\n",
+			  ctrl == ENABLE ? "en" : "dis", ret);
 		return ret;
+	}
 
 	return 0;
 }
@@ -150,10 +153,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 	}
 
 	ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
-	if (ret) {
-		DRM_ERROR("failed to enable VDO: %d\n", ret);
+	if (ret)
 		goto err_regulators_disable;
-	}
 
 	/* Switch access edp panel's edid through i2c */
 	ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
@@ -175,9 +176,7 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	int ret;
 
-	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
-	if (ret < 0)
-		DRM_ERROR("failed to disable VDO: %d\n", ret);
+	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
 
 	gpiod_set_value(ps_bridge->gpio_reset, 1);
 	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
-- 
2.27.0

Re: [PATCH 2/3] drm/bridge: ps8640: Print an error if VDO control fails

[Sam Ravnborg]

On Mon, Jun 15, 2020 at 10:53:19PM +0200, Enric Balletbo i Serra wrote:
> Print an error message inside ps8640_bridge_vdo_control() function when
> it fails so we can simplify a bit the callers, they will only need to
> check the error code.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
> 
>  drivers/gpu/drm/bridge/parade-ps8640.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index ce3e8b2da8c9b..9f7b7a9c53c52 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -82,8 +82,11 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>  	ret = i2c_smbus_write_i2c_block_data(client, PAGE3_SET_ADD,
>  					     sizeof(vdo_ctrl_buf),
>  					     vdo_ctrl_buf);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		DRM_ERROR("failed to %sable VDO: %d\n",
> +			  ctrl == ENABLE ? "en" : "dis", ret);
>  		return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -150,10 +153,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  	}
>  
>  	ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
> -	if (ret) {
> -		DRM_ERROR("failed to enable VDO: %d\n", ret);
> +	if (ret)
>  		goto err_regulators_disable;
> -	}
>  
>  	/* Switch access edp panel's edid through i2c */
>  	ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
> @@ -175,9 +176,7 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>  	int ret;
>  
> -	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> -	if (ret < 0)
> -		DRM_ERROR("failed to disable VDO: %d\n", ret);
> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>  
>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Enric Balletbo i Serra]

The get_edid() callback can be triggered anytime by an ioctl, i.e

  drm_mode_getconnector (ioctl)
    -> drm_helper_probe_single_connector_modes
       -> drm_bridge_connector_get_modes
          -> ps8640_bridge_get_edid

Actually if the bridge pre_enable() function was not called before
get_edid(), the driver will not be able to get the EDID properly and
display will not work until a second get_edid() call is issued and if
pre_enable() is called before. The side effect of this, for example, is
that you see anything when `Frecon` starts, neither the splash screen,
until the graphical session manager starts.

To fix this we need to make sure that all we need is enabled before
reading the EDID. This means the following:

1. If get_edid() is called before having the device powered we need to
   power on the device. In such case, the driver will power off again the
   device.

2. If get_edid() is called after having the device powered, all should
   just work. We added a powered flag in order to avoid recurrent calls
   to ps8640_bridge_poweron() and unneeded delays.

3. This seems to be specific for this device, but we need to make sure
   the panel is powered on before do a power on cycle on this device.
   Otherwise the device fails to retrieve the EDID.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 9f7b7a9c53c52..ca651480891df 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -65,6 +65,7 @@ struct ps8640 {
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
+	bool powered;
 };
 
 static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
@@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
 	return 0;
 }
 
-static void ps8640_pre_enable(struct drm_bridge *bridge)
+static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
 {
-	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
+	struct drm_bridge *panel;
 	unsigned long timeout;
 	int ret, status;
 
+	if (ps_bridge->powered)
+		return;
+
+	/*
+	 * That seems to be specific to this chip, and a weird behaviour, but
+	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
+	 * we don't do this, the chip is not able to read properly the EDID.
+	 */
+	panel = ps_bridge->panel_bridge;
+	if (panel->funcs && panel->funcs->pre_enable)
+		panel->funcs->pre_enable(panel);
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
 				    ps_bridge->supplies);
 	if (ret < 0) {
@@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 		goto err_regulators_disable;
 	}
 
+	ps_bridge->powered = true;
+
 	return;
 
 err_regulators_disable:
@@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 			       ps_bridge->supplies);
 }
 
-static void ps8640_post_disable(struct drm_bridge *bridge)
+static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
 {
-	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+	struct drm_bridge *panel;
 	int ret;
 
-	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
+	if (!ps_bridge->powered)
+		return;
 
 	gpiod_set_value(ps_bridge->gpio_reset, 1);
 	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
@@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
 				     ps_bridge->supplies);
 	if (ret < 0)
 		DRM_ERROR("cannot disable regulators %d\n", ret);
+
+	panel = ps_bridge->panel_bridge;
+	if (panel->funcs && panel->funcs->post_disable)
+		panel->funcs->post_disable(panel);
+
+	ps_bridge->powered = false;
+}
+
+static void ps8640_pre_enable(struct drm_bridge *bridge)
+{
+	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+	int ret;
+
+	ps8640_bridge_poweron(ps_bridge);
+
+	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
+	if (ret < 0)
+		ps8640_bridge_poweroff(ps_bridge);
+}
+
+static void ps8640_post_disable(struct drm_bridge *bridge)
+{
+	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+
+	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
+	ps8640_bridge_poweroff(ps_bridge);
 }
 
 static int ps8640_bridge_attach(struct drm_bridge *bridge,
@@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 					   struct drm_connector *connector)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+	bool poweroff = !ps_bridge->powered;
+	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.
+	 */
+	ps8640_bridge_poweron(ps_bridge);
 
-	return drm_get_edid(connector,
+	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)
+		ps8640_bridge_poweroff(ps_bridge);
+
+	return edid;
 }
 
 static const struct drm_bridge_funcs ps8640_bridge_funcs = {
-- 
2.27.0

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Sam Ravnborg]

Hi Enric.

On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
> The get_edid() callback can be triggered anytime by an ioctl, i.e
> 
>   drm_mode_getconnector (ioctl)
>     -> drm_helper_probe_single_connector_modes
>        -> drm_bridge_connector_get_modes
>           -> ps8640_bridge_get_edid
> 
> Actually if the bridge pre_enable() function was not called before
> get_edid(), the driver will not be able to get the EDID properly and
> display will not work until a second get_edid() call is issued and if
> pre_enable() is called before.
Is it correct to fix this in the driver?
Why not just fail and tell user-sapce to try again later?
(Dunno what error-code to use - there must be one).

Then we avoid complicating drivers fro somethign we really should
fix in user-space.

> The side effect of this, for example, is
> that you see anything when `Frecon` starts, neither the splash screen,
that you do not see ...

(Otherwise I do not parse the above).

> until the graphical session manager starts.
> 
> To fix this we need to make sure that all we need is enabled before
> reading the EDID. This means the following:
> 
> 1. If get_edid() is called before having the device powered we need to
>    power on the device. In such case, the driver will power off again the
>    device.
> 
> 2. If get_edid() is called after having the device powered, all should
>    just work. We added a powered flag in order to avoid recurrent calls
>    to ps8640_bridge_poweron() and unneeded delays.
> 
> 3. This seems to be specific for this device, but we need to make sure
>    the panel is powered on before do a power on cycle on this device.
>    Otherwise the device fails to retrieve the EDID.
Step 3. looks like an ugly hack too....

> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 9f7b7a9c53c52..ca651480891df 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -65,6 +65,7 @@ struct ps8640 {
>  	struct regulator_bulk_data supplies[2];
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_powerdown;
> +	bool powered;
>  };
>  
>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>  	return 0;
>  }
>  
> -static void ps8640_pre_enable(struct drm_bridge *bridge)
> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>  {
> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
> +	struct drm_bridge *panel;
>  	unsigned long timeout;
>  	int ret, status;
>  
> +	if (ps_bridge->powered)
> +		return;
> +
> +	/*
> +	 * That seems to be specific to this chip, and a weird behaviour, but
> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
> +	 * we don't do this, the chip is not able to read properly the EDID.
> +	 */
> +	panel = ps_bridge->panel_bridge;
> +	if (panel->funcs && panel->funcs->pre_enable)
> +		panel->funcs->pre_enable(panel);
> +
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>  				    ps_bridge->supplies);
>  	if (ret < 0) {
> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  		goto err_regulators_disable;
>  	}
>  
> +	ps_bridge->powered = true;
> +
>  	return;
>  
>  err_regulators_disable:
> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  			       ps_bridge->supplies);
>  }
>  
> -static void ps8640_post_disable(struct drm_bridge *bridge)
> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>  {
> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	struct drm_bridge *panel;
>  	int ret;
>  
> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	if (!ps_bridge->powered)
> +		return;
>  
>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>  				     ps_bridge->supplies);
>  	if (ret < 0)
>  		DRM_ERROR("cannot disable regulators %d\n", ret);
> +
> +	panel = ps_bridge->panel_bridge;
> +	if (panel->funcs && panel->funcs->post_disable)
> +		panel->funcs->post_disable(panel);
> +
> +	ps_bridge->powered = false;
> +}
> +
> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	int ret;
> +
> +	ps8640_bridge_poweron(ps_bridge);
> +
> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	if (ret < 0)
> +		ps8640_bridge_poweroff(ps_bridge);
> +}
> +
> +static void ps8640_post_disable(struct drm_bridge *bridge)
> +{
> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +
> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	ps8640_bridge_poweroff(ps_bridge);
>  }
>  
>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  					   struct drm_connector *connector)
>  {
>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	bool poweroff = !ps_bridge->powered;
> +	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.
> +	 */
> +	ps8640_bridge_poweron(ps_bridge);
>  
> -	return drm_get_edid(connector,
> +	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)
> +		ps8640_bridge_poweroff(ps_bridge);
> +
> +	return edid;
>  }
>  
>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Enric Balletbo i Serra]

Hi Sam,

Many thanks for your feedback. See my answers below.

On 20/6/20 23:42, Sam Ravnborg wrote:
> Hi Enric.
> 
> On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
>> The get_edid() callback can be triggered anytime by an ioctl, i.e
>>
>>   drm_mode_getconnector (ioctl)
>>     -> drm_helper_probe_single_connector_modes
>>        -> drm_bridge_connector_get_modes
>>           -> ps8640_bridge_get_edid
>>
>> Actually if the bridge pre_enable() function was not called before
>> get_edid(), the driver will not be able to get the EDID properly and
>> display will not work until a second get_edid() call is issued and if
>> pre_enable() is called before.
> Is it correct to fix this in the driver?
> Why not just fail and tell user-sapce to try again later?
> (Dunno what error-code to use - there must be one).
> 

My undestanding, I might be wrong, is that userspace should don't know which
bits, regulators, etc, are needed to get the EDID with an ioctl. Is the kernel
that should make sure that all is set properly (the required regulators, etc)
when userspace wants to read the EDID.

> Then we avoid complicating drivers fro somethign we really should
> fix in user-space.
> 
>> The side effect of this, for example, is
>> that you see anything when `Frecon` starts, neither the splash screen,
> that you do not see ...
> 
> (Otherwise I do not parse the above).
> 
>> until the graphical session manager starts.
>>
>> To fix this we need to make sure that all we need is enabled before
>> reading the EDID. This means the following:
>>
>> 1. If get_edid() is called before having the device powered we need to
>>    power on the device. In such case, the driver will power off again the
>>    device.
>>
>> 2. If get_edid() is called after having the device powered, all should
>>    just work. We added a powered flag in order to avoid recurrent calls
>>    to ps8640_bridge_poweron() and unneeded delays.
>>
>> 3. This seems to be specific for this device, but we need to make sure
>>    the panel is powered on before do a power on cycle on this device.
>>    Otherwise the device fails to retrieve the EDID.
> Step 3. looks like an ugly hack too....
> 

It is, but I don't have enough hardware details to be able to answer why we need
to do this. What is well tested is that, if I don't power the panel before
powering on the bridge, it doesn't get a proper EDID. Seems that when the bridge
goes up, the firmware tries to read the EDID and caches somehow. Well not sure.

>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>>  1 file changed, 73 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index 9f7b7a9c53c52..ca651480891df 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -65,6 +65,7 @@ struct ps8640 {
>>  	struct regulator_bulk_data supplies[2];
>>  	struct gpio_desc *gpio_reset;
>>  	struct gpio_desc *gpio_powerdown;
>> +	bool powered;
>>  };
>>  
>>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>>  	return 0;
>>  }
>>  
>> -static void ps8640_pre_enable(struct drm_bridge *bridge)
>> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>>  {
>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
>> +	struct drm_bridge *panel;
>>  	unsigned long timeout;
>>  	int ret, status;
>>  
>> +	if (ps_bridge->powered)
>> +		return;
>> +
>> +	/*
>> +	 * That seems to be specific to this chip, and a weird behaviour, but
>> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
>> +	 * we don't do this, the chip is not able to read properly the EDID.
>> +	 */
>> +	panel = ps_bridge->panel_bridge;
>> +	if (panel->funcs && panel->funcs->pre_enable)
>> +		panel->funcs->pre_enable(panel);
>> +
>>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>>  				    ps_bridge->supplies);
>>  	if (ret < 0) {
>> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>  		goto err_regulators_disable;
>>  	}
>>  
>> +	ps_bridge->powered = true;
>> +
>>  	return;
>>  
>>  err_regulators_disable:
>> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>  			       ps_bridge->supplies);
>>  }
>>  
>> -static void ps8640_post_disable(struct drm_bridge *bridge)
>> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>>  {
>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +	struct drm_bridge *panel;
>>  	int ret;
>>  
>> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>> +	if (!ps_bridge->powered)
>> +		return;
>>  
>>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>>  				     ps_bridge->supplies);
>>  	if (ret < 0)
>>  		DRM_ERROR("cannot disable regulators %d\n", ret);
>> +
>> +	panel = ps_bridge->panel_bridge;
>> +	if (panel->funcs && panel->funcs->post_disable)
>> +		panel->funcs->post_disable(panel);
>> +
>> +	ps_bridge->powered = false;
>> +}
>> +
>> +static void ps8640_pre_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +	int ret;
>> +
>> +	ps8640_bridge_poweron(ps_bridge);
>> +
>> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>> +	if (ret < 0)
>> +		ps8640_bridge_poweroff(ps_bridge);
>> +}
>> +
>> +static void ps8640_post_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +
>> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>> +	ps8640_bridge_poweroff(ps_bridge);
>>  }
>>  
>>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
>> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>>  					   struct drm_connector *connector)
>>  {
>>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +	bool poweroff = !ps_bridge->powered;
>> +	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.
>> +	 */
>> +	ps8640_bridge_poweron(ps_bridge);
>>  
>> -	return drm_get_edid(connector,
>> +	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)
>> +		ps8640_bridge_poweroff(ps_bridge);
>> +
>> +	return edid;
>>  }
>>  
>>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Sam Ravnborg]

Hi Enric.

On Tue, Jun 23, 2020 at 05:16:43PM +0200, Enric Balletbo i Serra wrote:
> Hi Sam,
> 
> Many thanks for your feedback. See my answers below.
> 
> On 20/6/20 23:42, Sam Ravnborg wrote:
> > Hi Enric.
> > 
> > On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
> >> The get_edid() callback can be triggered anytime by an ioctl, i.e
> >>
> >>   drm_mode_getconnector (ioctl)
> >>     -> drm_helper_probe_single_connector_modes
> >>        -> drm_bridge_connector_get_modes
> >>           -> ps8640_bridge_get_edid
> >>
> >> Actually if the bridge pre_enable() function was not called before
> >> get_edid(), the driver will not be able to get the EDID properly and
> >> display will not work until a second get_edid() call is issued and if
> >> pre_enable() is called before.
> > Is it correct to fix this in the driver?
> > Why not just fail and tell user-sapce to try again later?
> > (Dunno what error-code to use - there must be one).
> > 
> 
> My undestanding, I might be wrong, is that userspace should don't know which
> bits, regulators, etc, are needed to get the EDID with an ioctl. Is the kernel
> that should make sure that all is set properly (the required regulators, etc)
> when userspace wants to read the EDID.

The idea I suggest is that the kernel side should just error
out and let user-space retry later.
So we avoid all the extra logic in the kernel because userspace wants
info before the HW is ready.
And userspace shall anyway deal with the ioctl that fails.

	Sam

> 
> > Then we avoid complicating drivers fro somethign we really should
> > fix in user-space.
> > 
> >> The side effect of this, for example, is
> >> that you see anything when `Frecon` starts, neither the splash screen,
> > that you do not see ...
> > 
> > (Otherwise I do not parse the above).
> > 
> >> until the graphical session manager starts.
> >>
> >> To fix this we need to make sure that all we need is enabled before
> >> reading the EDID. This means the following:
> >>
> >> 1. If get_edid() is called before having the device powered we need to
> >>    power on the device. In such case, the driver will power off again the
> >>    device.
> >>
> >> 2. If get_edid() is called after having the device powered, all should
> >>    just work. We added a powered flag in order to avoid recurrent calls
> >>    to ps8640_bridge_poweron() and unneeded delays.
> >>
> >> 3. This seems to be specific for this device, but we need to make sure
> >>    the panel is powered on before do a power on cycle on this device.
> >>    Otherwise the device fails to retrieve the EDID.
> > Step 3. looks like an ugly hack too....
> > 
> 
> It is, but I don't have enough hardware details to be able to answer why we need
> to do this. What is well tested is that, if I don't power the panel before
> powering on the bridge, it doesn't get a proper EDID. Seems that when the bridge
> goes up, the firmware tries to read the EDID and caches somehow. Well not sure.
> 
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
> >>  1 file changed, 73 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> index 9f7b7a9c53c52..ca651480891df 100644
> >> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> >> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> @@ -65,6 +65,7 @@ struct ps8640 {
> >>  	struct regulator_bulk_data supplies[2];
> >>  	struct gpio_desc *gpio_reset;
> >>  	struct gpio_desc *gpio_powerdown;
> >> +	bool powered;
> >>  };
> >>  
> >>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> >> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
> >>  	return 0;
> >>  }
> >>  
> >> -static void ps8640_pre_enable(struct drm_bridge *bridge)
> >> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
> >>  {
> >> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> >>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
> >> +	struct drm_bridge *panel;
> >>  	unsigned long timeout;
> >>  	int ret, status;
> >>  
> >> +	if (ps_bridge->powered)
> >> +		return;
> >> +
> >> +	/*
> >> +	 * That seems to be specific to this chip, and a weird behaviour, but
> >> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
> >> +	 * we don't do this, the chip is not able to read properly the EDID.
> >> +	 */
> >> +	panel = ps_bridge->panel_bridge;
> >> +	if (panel->funcs && panel->funcs->pre_enable)
> >> +		panel->funcs->pre_enable(panel);
> >> +
> >>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> >>  				    ps_bridge->supplies);
> >>  	if (ret < 0) {
> >> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> >>  		goto err_regulators_disable;
> >>  	}
> >>  
> >> +	ps_bridge->powered = true;
> >> +
> >>  	return;
> >>  
> >>  err_regulators_disable:
> >> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> >>  			       ps_bridge->supplies);
> >>  }
> >>  
> >> -static void ps8640_post_disable(struct drm_bridge *bridge)
> >> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> >>  {
> >> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> >> +	struct drm_bridge *panel;
> >>  	int ret;
> >>  
> >> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> >> +	if (!ps_bridge->powered)
> >> +		return;
> >>  
> >>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
> >>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> >> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
> >>  				     ps_bridge->supplies);
> >>  	if (ret < 0)
> >>  		DRM_ERROR("cannot disable regulators %d\n", ret);
> >> +
> >> +	panel = ps_bridge->panel_bridge;
> >> +	if (panel->funcs && panel->funcs->post_disable)
> >> +		panel->funcs->post_disable(panel);
> >> +
> >> +	ps_bridge->powered = false;
> >> +}
> >> +
> >> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> >> +{
> >> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> >> +	int ret;
> >> +
> >> +	ps8640_bridge_poweron(ps_bridge);
> >> +
> >> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> >> +	if (ret < 0)
> >> +		ps8640_bridge_poweroff(ps_bridge);
> >> +}
> >> +
> >> +static void ps8640_post_disable(struct drm_bridge *bridge)
> >> +{
> >> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> >> +
> >> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> >> +	ps8640_bridge_poweroff(ps_bridge);
> >>  }
> >>  
> >>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
> >> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> >>  					   struct drm_connector *connector)
> >>  {
> >>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> >> +	bool poweroff = !ps_bridge->powered;
> >> +	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.
> >> +	 */
> >> +	ps8640_bridge_poweron(ps_bridge);
> >>  
> >> -	return drm_get_edid(connector,
> >> +	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)
> >> +		ps8640_bridge_poweroff(ps_bridge);
> >> +
> >> +	return edid;
> >>  }
> >>  
> >>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> >> -- 
> >> 2.27.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Enric Balletbo i Serra]

Hi Sam,

On 24/6/20 9:07, Sam Ravnborg wrote:
> Hi Enric.
> 
> On Tue, Jun 23, 2020 at 05:16:43PM +0200, Enric Balletbo i Serra wrote:
>> Hi Sam,
>>
>> Many thanks for your feedback. See my answers below.
>>
>> On 20/6/20 23:42, Sam Ravnborg wrote:
>>> Hi Enric.
>>>
>>> On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
>>>> The get_edid() callback can be triggered anytime by an ioctl, i.e
>>>>
>>>>   drm_mode_getconnector (ioctl)
>>>>     -> drm_helper_probe_single_connector_modes
>>>>        -> drm_bridge_connector_get_modes
>>>>           -> ps8640_bridge_get_edid
>>>>
>>>> Actually if the bridge pre_enable() function was not called before
>>>> get_edid(), the driver will not be able to get the EDID properly and
>>>> display will not work until a second get_edid() call is issued and if
>>>> pre_enable() is called before.
>>> Is it correct to fix this in the driver?
>>> Why not just fail and tell user-sapce to try again later?
>>> (Dunno what error-code to use - there must be one).
>>>
>>
>> My undestanding, I might be wrong, is that userspace should don't know which
>> bits, regulators, etc, are needed to get the EDID with an ioctl. Is the kernel
>> that should make sure that all is set properly (the required regulators, etc)
>> when userspace wants to read the EDID.
> 
> The idea I suggest is that the kernel side should just error
> out and let user-space retry later.

I see, I got it.

> So we avoid all the extra logic in the kernel because userspace wants
> info before the HW is ready.
> And userspace shall anyway deal with the ioctl that fails.
> 

My doubt here is, at which point the kernel driver should leave the hardware in
a state that getting the EDID should work? After the bridge pre_enable() or
enable() calls? After has been proved? AFAIK the bridge pre_enable() and
enable() calls are only about enable the stream out and in most cases you can
get the EDID without having to call pre_enable() and enable() before.

Thanks,
 Enric


> 	Sam
> 
>>
>>> Then we avoid complicating drivers fro somethign we really should
>>> fix in user-space.
>>>
>>>> The side effect of this, for example, is
>>>> that you see anything when `Frecon` starts, neither the splash screen,
>>> that you do not see ...
>>>
>>> (Otherwise I do not parse the above).
>>>
>>>> until the graphical session manager starts.
>>>>
>>>> To fix this we need to make sure that all we need is enabled before
>>>> reading the EDID. This means the following:
>>>>
>>>> 1. If get_edid() is called before having the device powered we need to
>>>>    power on the device. In such case, the driver will power off again the
>>>>    device.
>>>>
>>>> 2. If get_edid() is called after having the device powered, all should
>>>>    just work. We added a powered flag in order to avoid recurrent calls
>>>>    to ps8640_bridge_poweron() and unneeded delays.
>>>>
>>>> 3. This seems to be specific for this device, but we need to make sure
>>>>    the panel is powered on before do a power on cycle on this device.
>>>>    Otherwise the device fails to retrieve the EDID.
>>> Step 3. looks like an ugly hack too....
>>>
>>
>> It is, but I don't have enough hardware details to be able to answer why we need
>> to do this. What is well tested is that, if I don't power the panel before
>> powering on the bridge, it doesn't get a proper EDID. Seems that when the bridge
>> goes up, the firmware tries to read the EDID and caches somehow. Well not sure.
>>
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>>>>  1 file changed, 73 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> index 9f7b7a9c53c52..ca651480891df 100644
>>>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> @@ -65,6 +65,7 @@ struct ps8640 {
>>>>  	struct regulator_bulk_data supplies[2];
>>>>  	struct gpio_desc *gpio_reset;
>>>>  	struct gpio_desc *gpio_powerdown;
>>>> +	bool powered;
>>>>  };
>>>>  
>>>>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>>>> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>>>>  {
>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
>>>> +	struct drm_bridge *panel;
>>>>  	unsigned long timeout;
>>>>  	int ret, status;
>>>>  
>>>> +	if (ps_bridge->powered)
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * That seems to be specific to this chip, and a weird behaviour, but
>>>> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
>>>> +	 * we don't do this, the chip is not able to read properly the EDID.
>>>> +	 */
>>>> +	panel = ps_bridge->panel_bridge;
>>>> +	if (panel->funcs && panel->funcs->pre_enable)
>>>> +		panel->funcs->pre_enable(panel);
>>>> +
>>>>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>>>>  				    ps_bridge->supplies);
>>>>  	if (ret < 0) {
>>>> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>  		goto err_regulators_disable;
>>>>  	}
>>>>  
>>>> +	ps_bridge->powered = true;
>>>> +
>>>>  	return;
>>>>  
>>>>  err_regulators_disable:
>>>> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>  			       ps_bridge->supplies);
>>>>  }
>>>>  
>>>> -static void ps8640_post_disable(struct drm_bridge *bridge)
>>>> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>>>>  {
>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	struct drm_bridge *panel;
>>>>  	int ret;
>>>>  
>>>> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	if (!ps_bridge->powered)
>>>> +		return;
>>>>  
>>>>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>>>>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>>>> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>>>>  				     ps_bridge->supplies);
>>>>  	if (ret < 0)
>>>>  		DRM_ERROR("cannot disable regulators %d\n", ret);
>>>> +
>>>> +	panel = ps_bridge->panel_bridge;
>>>> +	if (panel->funcs && panel->funcs->post_disable)
>>>> +		panel->funcs->post_disable(panel);
>>>> +
>>>> +	ps_bridge->powered = false;
>>>> +}
>>>> +
>>>> +static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	int ret;
>>>> +
>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>> +
>>>> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	if (ret < 0)
>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>> +}
>>>> +
>>>> +static void ps8640_post_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +
>>>> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	ps8640_bridge_poweroff(ps_bridge);
>>>>  }
>>>>  
>>>>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>>> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>>>>  					   struct drm_connector *connector)
>>>>  {
>>>>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	bool poweroff = !ps_bridge->powered;
>>>> +	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.
>>>> +	 */
>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>>  
>>>> -	return drm_get_edid(connector,
>>>> +	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)
>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>> +
>>>> +	return edid;
>>>>  }
>>>>  
>>>>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>>>> -- 
>>>> 2.27.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Enric Balletbo i Serra]

Dear drm_bridge maintainers,

It's been a while since I send these patches, and I'd like to find a proper
solution.

On 25/6/20 11:21, Enric Balletbo i Serra wrote:
> Hi Sam,
> 
> On 24/6/20 9:07, Sam Ravnborg wrote:
>> Hi Enric.
>>
>> On Tue, Jun 23, 2020 at 05:16:43PM +0200, Enric Balletbo i Serra wrote:
>>> Hi Sam,
>>>
>>> Many thanks for your feedback. See my answers below.
>>>
>>> On 20/6/20 23:42, Sam Ravnborg wrote:
>>>> Hi Enric.
>>>>
>>>> On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
>>>>> The get_edid() callback can be triggered anytime by an ioctl, i.e
>>>>>
>>>>>   drm_mode_getconnector (ioctl)
>>>>>     -> drm_helper_probe_single_connector_modes
>>>>>        -> drm_bridge_connector_get_modes
>>>>>           -> ps8640_bridge_get_edid
>>>>>
>>>>> Actually if the bridge pre_enable() function was not called before
>>>>> get_edid(), the driver will not be able to get the EDID properly and
>>>>> display will not work until a second get_edid() call is issued and if
>>>>> pre_enable() is called before.
>>>> Is it correct to fix this in the driver?
>>>> Why not just fail and tell user-sapce to try again later?
>>>> (Dunno what error-code to use - there must be one).
>>>>
>>>
>>> My undestanding, I might be wrong, is that userspace should don't know which
>>> bits, regulators, etc, are needed to get the EDID with an ioctl. Is the kernel
>>> that should make sure that all is set properly (the required regulators, etc)
>>> when userspace wants to read the EDID.
>>
>> The idea I suggest is that the kernel side should just error
>> out and let user-space retry later.
> 
> I see, I got it.
> 
>> So we avoid all the extra logic in the kernel because userspace wants
>> info before the HW is ready.
>> And userspace shall anyway deal with the ioctl that fails.
>>
> 
> My doubt here is, at which point the kernel driver should leave the hardware in
> a state that getting the EDID should work? After the bridge pre_enable() or
> enable() calls? After has been proved? AFAIK the bridge pre_enable() and
> enable() calls are only about enable the stream out and in most cases you can
> get the EDID without having to call pre_enable() and enable() before.
> 

Let me reformulate the question for if it was not clear.

What I did is be able to read the EDID every time userspace asks for it (so
kernel enables all the required) and Sam is proposing to just fail if all is not
setup. I can obviously do this but my question is, at which point I should leave
all the logic enabled to be able to read the EDID (after probe?, after
pre_enable, after enable?) It is not clear for me from the API.

Thanks,
 Enric


> Thanks,
>  Enric
> 
> 
>> 	Sam
>>
>>>
>>>> Then we avoid complicating drivers fro somethign we really should
>>>> fix in user-space.
>>>>
>>>>> The side effect of this, for example, is
>>>>> that you see anything when `Frecon` starts, neither the splash screen,
>>>> that you do not see ...
>>>>
>>>> (Otherwise I do not parse the above).
>>>>
>>>>> until the graphical session manager starts.
>>>>>
>>>>> To fix this we need to make sure that all we need is enabled before
>>>>> reading the EDID. This means the following:
>>>>>
>>>>> 1. If get_edid() is called before having the device powered we need to
>>>>>    power on the device. In such case, the driver will power off again the
>>>>>    device.
>>>>>
>>>>> 2. If get_edid() is called after having the device powered, all should
>>>>>    just work. We added a powered flag in order to avoid recurrent calls
>>>>>    to ps8640_bridge_poweron() and unneeded delays.
>>>>>
>>>>> 3. This seems to be specific for this device, but we need to make sure
>>>>>    the panel is powered on before do a power on cycle on this device.
>>>>>    Otherwise the device fails to retrieve the EDID.
>>>> Step 3. looks like an ugly hack too....
>>>>
>>>
>>> It is, but I don't have enough hardware details to be able to answer why we need
>>> to do this. What is well tested is that, if I don't power the panel before
>>> powering on the bridge, it doesn't get a proper EDID. Seems that when the bridge
>>> goes up, the firmware tries to read the EDID and caches somehow. Well not sure.
>>>
>>>>>
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>>> ---
>>>>>
>>>>>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>>>>>  1 file changed, 73 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>>> index 9f7b7a9c53c52..ca651480891df 100644
>>>>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>>>>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>>> @@ -65,6 +65,7 @@ struct ps8640 {
>>>>>  	struct regulator_bulk_data supplies[2];
>>>>>  	struct gpio_desc *gpio_reset;
>>>>>  	struct gpio_desc *gpio_powerdown;
>>>>> +	bool powered;
>>>>>  };
>>>>>  
>>>>>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>>>>> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> -static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>>>>>  {
>>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
>>>>> +	struct drm_bridge *panel;
>>>>>  	unsigned long timeout;
>>>>>  	int ret, status;
>>>>>  
>>>>> +	if (ps_bridge->powered)
>>>>> +		return;
>>>>> +
>>>>> +	/*
>>>>> +	 * That seems to be specific to this chip, and a weird behaviour, but
>>>>> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
>>>>> +	 * we don't do this, the chip is not able to read properly the EDID.
>>>>> +	 */
>>>>> +	panel = ps_bridge->panel_bridge;
>>>>> +	if (panel->funcs && panel->funcs->pre_enable)
>>>>> +		panel->funcs->pre_enable(panel);
>>>>> +
>>>>>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>>>>>  				    ps_bridge->supplies);
>>>>>  	if (ret < 0) {
>>>>> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>>  		goto err_regulators_disable;
>>>>>  	}
>>>>>  
>>>>> +	ps_bridge->powered = true;
>>>>> +
>>>>>  	return;
>>>>>  
>>>>>  err_regulators_disable:
>>>>> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>>  			       ps_bridge->supplies);
>>>>>  }
>>>>>  
>>>>> -static void ps8640_post_disable(struct drm_bridge *bridge)
>>>>> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>>>>>  {
>>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>> +	struct drm_bridge *panel;
>>>>>  	int ret;
>>>>>  
>>>>> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>>> +	if (!ps_bridge->powered)
>>>>> +		return;
>>>>>  
>>>>>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>>>>>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>>>>> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>>>>>  				     ps_bridge->supplies);
>>>>>  	if (ret < 0)
>>>>>  		DRM_ERROR("cannot disable regulators %d\n", ret);
>>>>> +
>>>>> +	panel = ps_bridge->panel_bridge;
>>>>> +	if (panel->funcs && panel->funcs->post_disable)
>>>>> +		panel->funcs->post_disable(panel);
>>>>> +
>>>>> +	ps_bridge->powered = false;
>>>>> +}
>>>>> +
>>>>> +static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>> +{
>>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>> +	int ret;
>>>>> +
>>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>>> +
>>>>> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>>> +	if (ret < 0)
>>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>>> +}
>>>>> +
>>>>> +static void ps8640_post_disable(struct drm_bridge *bridge)
>>>>> +{
>>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>> +
>>>>> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>>> +	ps8640_bridge_poweroff(ps_bridge);
>>>>>  }
>>>>>  
>>>>>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>>>> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>>>>>  					   struct drm_connector *connector)
>>>>>  {
>>>>>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>> +	bool poweroff = !ps_bridge->powered;
>>>>> +	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.
>>>>> +	 */
>>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>>>  
>>>>> -	return drm_get_edid(connector,
>>>>> +	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)
>>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>>> +
>>>>> +	return edid;
>>>>>  }
>>>>>  
>>>>>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>>>>> -- 
>>>>> 2.27.0
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Sam Ravnborg]

Hi Enric.

> 
> Let me reformulate the question for if it was not clear.
> 
> What I did is be able to read the EDID every time userspace asks for it (so
> kernel enables all the required) and Sam is proposing to just fail if all is not
> setup. I can obviously do this but my question is, at which point I should leave
> all the logic enabled to be able to read the EDID (after probe?, after
> pre_enable, after enable?) It is not clear for me from the API.

I am not clear if my suggestion is a good suggestion.

I recall I saw something similar in another bridge driver.

If no-one else chimes in then go ahead with your original approach
where there is less restriction when it is possible to read edid.

	Sam

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Sam Ravnborg]

Hi Enric.

On Fri, Aug 21, 2020 at 01:38:09PM +0200, Sam Ravnborg wrote:
> Hi Enric.
> 
> > 
> > Let me reformulate the question for if it was not clear.
> > 
> > What I did is be able to read the EDID every time userspace asks for it (so
> > kernel enables all the required) and Sam is proposing to just fail if all is not
> > setup. I can obviously do this but my question is, at which point I should leave
> > all the logic enabled to be able to read the EDID (after probe?, after
> > pre_enable, after enable?) It is not clear for me from the API.
> 
> I am not clear if my suggestion is a good suggestion.
> 
> I recall I saw something similar in another bridge driver.

I have noew checked - and there is several bridge drivers that
do a power_on to read get_edid - so I was mistaken. Sorry!

	Sam

Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling

[Sam Ravnborg]

On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
> The get_edid() callback can be triggered anytime by an ioctl, i.e
> 
>   drm_mode_getconnector (ioctl)
>     -> drm_helper_probe_single_connector_modes
>        -> drm_bridge_connector_get_modes
>           -> ps8640_bridge_get_edid
> 
> Actually if the bridge pre_enable() function was not called before
> get_edid(), the driver will not be able to get the EDID properly and
> display will not work until a second get_edid() call is issued and if
> pre_enable() is called before. The side effect of this, for example, is
> that you see anything when `Frecon` starts, neither the splash screen,
> until the graphical session manager starts.
> 
> To fix this we need to make sure that all we need is enabled before
> reading the EDID. This means the following:
> 
> 1. If get_edid() is called before having the device powered we need to
>    power on the device. In such case, the driver will power off again the
>    device.
> 
> 2. If get_edid() is called after having the device powered, all should
>    just work. We added a powered flag in order to avoid recurrent calls
>    to ps8640_bridge_poweron() and unneeded delays.
> 
> 3. This seems to be specific for this device, but we need to make sure
>    the panel is powered on before do a power on cycle on this device.
>    Otherwise the device fails to retrieve the EDID.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 9f7b7a9c53c52..ca651480891df 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -65,6 +65,7 @@ struct ps8640 {
>  	struct regulator_bulk_data supplies[2];
>  	struct gpio_desc *gpio_reset;
>  	struct gpio_desc *gpio_powerdown;
> +	bool powered;
>  };
>  
>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>  	return 0;
>  }
>  
> -static void ps8640_pre_enable(struct drm_bridge *bridge)
> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>  {
> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
> +	struct drm_bridge *panel;
>  	unsigned long timeout;
>  	int ret, status;
>  
> +	if (ps_bridge->powered)
> +		return;
> +
> +	/*
> +	 * That seems to be specific to this chip, and a weird behaviour, but
> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
> +	 * we don't do this, the chip is not able to read properly the EDID.
> +	 */
> +	panel = ps_bridge->panel_bridge;
> +	if (panel->funcs && panel->funcs->pre_enable)
> +		panel->funcs->pre_enable(panel);
> +
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>  				    ps_bridge->supplies);
>  	if (ret < 0) {
> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  		goto err_regulators_disable;
>  	}
>  
> +	ps_bridge->powered = true;
> +
>  	return;
>  
>  err_regulators_disable:
> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  			       ps_bridge->supplies);
>  }
>  
> -static void ps8640_post_disable(struct drm_bridge *bridge)
> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>  {
> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	struct drm_bridge *panel;
>  	int ret;
>  
> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	if (!ps_bridge->powered)
> +		return;
>  
>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>  				     ps_bridge->supplies);
>  	if (ret < 0)
>  		DRM_ERROR("cannot disable regulators %d\n", ret);
> +
> +	panel = ps_bridge->panel_bridge;
> +	if (panel->funcs && panel->funcs->post_disable)
> +		panel->funcs->post_disable(panel);
> +
> +	ps_bridge->powered = false;
> +}
> +
> +static void ps8640_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	int ret;
> +
> +	ps8640_bridge_poweron(ps_bridge);
> +
> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	if (ret < 0)
> +		ps8640_bridge_poweroff(ps_bridge);
> +}
> +
> +static void ps8640_post_disable(struct drm_bridge *bridge)
> +{
> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +
> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
> +	ps8640_bridge_poweroff(ps_bridge);
>  }

Hi Enric.

Would is be possible to use drm_bridge_chain_pre_enable()
and drm_bridge_chain_post_disable() rather than hardcoding
the dependencies here.
The the already present logic to handle a bridged chain is used
when pre_enable and post_disable.
Seems better to use the present logic to handle bridged chains if
possible. And it should simplify this logic a bit too.

	Sam

>  
>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  					   struct drm_connector *connector)
>  {
>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +	bool poweroff = !ps_bridge->powered;
> +	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.
> +	 */
> +	ps8640_bridge_poweron(ps_bridge);
>  
> -	return drm_get_edid(connector,
> +	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)
> +		ps8640_bridge_poweroff(ps_bridge);
> +
> +	return edid;
>  }
>  
>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> -- 
> 2.27.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel