Re: [PATCH v2 5/5] drm/bridge: ps8640: Rework power state handling
Hi Sam,
Thanks for your comments.
On 26/8/20 20:46, Sam Ravnborg wrote:
> Hi Enric.
>
> On Wed, Aug 26, 2020 at 10:15:26AM +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
>> ---
>>
>> Changes in v2:
>> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam Ravnborg)
>>
>> drivers/gpu/drm/bridge/parade-ps8640.c | 64 +++---
>> 1 file changed, 58 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>> b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index 9f7b7a9c53c5..c5d76e209bda 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,15 @@ 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];
>> unsigned long timeout;
>> int ret, status;
>>
>> +if (ps_bridge->powered)
>> +return;
>> +
>> ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>> ps_bridge->supplies);
>> if (ret < 0) {
>> @@ -164,6 +167,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>> goto err_regulators_disable;
>> }
>>
>> +ps_bridge->powered = true;
>> +
>> return;
>>
>> err_regulators_disable:
>> @@ -171,12 +176,12 @@ 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);
>> 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 +189,28 @@ static void ps8640_post_disable(struct drm_bridge
>> *bridge)
>> ps_bridge->supplies);
>> if (ret < 0)
>> DRM_ERROR("cannot disable regulators %d\n", ret);
>> +
>> +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);
>> +}
>
> The impleimentation of ps8640_bridge_poweron() versus
> ps8640_bridge_poweroff() is confusing.
>
> ps8640_bridge_poweron() includes ps8640_bridge_vdo_control(.., ENABLE),
> but ps8640_bridge_poweroff() does not include
> ps8640_bridge_vdo_control(..., DISABLE).
>
> This is inconsistent and confusing. At least it was for me when
> reviewing. Can this be improved - or maybe just use naming that does not
> indicate they are the reverse of each other?
>
Right, I think I can implement reverse of each other. So I'll send an updated
series.
Thanks,
Enric
>> +
>> +static void
Re: [PATCH v2 5/5] drm/bridge: ps8640: Rework power state handling
On Wed, 26 Aug 2020 10:15:26 +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
> ---
>
> Changes in v2:
> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam
> Ravnborg)
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 64
> +++--- 1 file changed, 58 insertions(+), 6
> deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c index
> 9f7b7a9c53c5..c5d76e209bda 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,15 @@ 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];
> unsigned long timeout;
> int ret, status;
>
> + if (ps_bridge->powered)
> + return;
> +
> ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> ps_bridge->supplies);
> if (ret < 0) {
> @@ -164,6 +167,8 @@ static void ps8640_pre_enable(struct drm_bridge
> *bridge) goto err_regulators_disable;
> }
>
> + ps_bridge->powered = true;
> +
> return;
>
> err_regulators_disable:
> @@ -171,12 +176,12 @@ 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);
> 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 +189,28 @@ static void ps8640_post_disable(struct
> drm_bridge *bridge) ps_bridge->supplies);
> if (ret < 0)
> DRM_ERROR("cannot disable regulators %d\n", ret);
> +
> + 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 +276,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
>
Re: [PATCH v2 5/5] drm/bridge: ps8640: Rework power state handling
Hi Enric.
On Wed, Aug 26, 2020 at 10:15:26AM +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
> ---
>
> Changes in v2:
> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam Ravnborg)
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 64 +++---
> 1 file changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 9f7b7a9c53c5..c5d76e209bda 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,15 @@ 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];
> unsigned long timeout;
> int ret, status;
>
> + if (ps_bridge->powered)
> + return;
> +
> ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
> ps_bridge->supplies);
> if (ret < 0) {
> @@ -164,6 +167,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> goto err_regulators_disable;
> }
>
> + ps_bridge->powered = true;
> +
> return;
>
> err_regulators_disable:
> @@ -171,12 +176,12 @@ 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);
> 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 +189,28 @@ static void ps8640_post_disable(struct drm_bridge
> *bridge)
>ps_bridge->supplies);
> if (ret < 0)
> DRM_ERROR("cannot disable regulators %d\n", ret);
> +
> + 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);
> +}
The impleimentation of ps8640_bridge_poweron() versus
ps8640_bridge_poweroff() is confusing.
ps8640_bridge_poweron() includes ps8640_bridge_vdo_control(.., ENABLE),
but ps8640_bridge_poweroff() does not include
ps8640_bridge_vdo_control(..., DISABLE).
This is inconsistent and confusing. At least it was for me when
reviewing. Can this be improved - or maybe just use naming that does not
indicate they are the reverse of each other?
> +
> +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,
> @@
