* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-27 8:59 ` [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling Enric Balletbo i Serra
@ 2020-08-27 19:34 ` kernel test robot
2020-08-31 9:49 ` Enric Balletbo i Serra
2020-08-31 9:32 ` Bilal Wasim
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: kernel test robot @ 2020-08-27 19:34 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kernel
Cc: kbuild-all, Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie
[-- Attachment #1: Type: text/plain, Size: 1822 bytes --]
Hi Enric,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[cannot apply to linux/master drm-intel/for-linux-next linus/master v5.9-rc2]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Enric-Balletbo-i-Serra/drm-bridge-ps8640-Make-sure-all-needed-is-powered-to-get-the-EDID/20200827-170009
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: riscv-randconfig-r001-20200827 (attached as .config)
compiler: riscv32-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
<< WARNING: modpost: vmlinux.o(__ex_table+0xed0): Section mismatch in reference from the (unknown reference) (unknown) to the variable .debug_str:.LASF1611
>> WARNING: modpost: vmlinux.o(__ex_table+0xf40): Section mismatch in reference from the (unknown reference) (unknown) to the variable .debug_str:.LASF1482
FATAL: modpost: The relocation at __ex_table+0xf40 references
section ".debug_str" which is not executable, IOW
it is not possible for the kernel to fault
at that address. Something is seriously wrong
and should be fixed.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32596 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-27 19:34 ` kernel test robot
@ 2020-08-31 9:49 ` Enric Balletbo i Serra
0 siblings, 0 replies; 10+ messages in thread
From: Enric Balletbo i Serra @ 2020-08-31 9:49 UTC (permalink / raw)
To: kernel test robot, linux-kernel
Cc: kbuild-all, Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie
Hi,
On 27/8/20 21:34, kernel test robot wrote:
> Hi Enric,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on drm-tip/drm-tip]
> [cannot apply to linux/master drm-intel/for-linux-next linus/master v5.9-rc2]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Enric-Balletbo-i-Serra/drm-bridge-ps8640-Make-sure-all-needed-is-powered-to-get-the-EDID/20200827-170009
> base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
> config: riscv-randconfig-r001-20200827 (attached as .config)
> compiler: riscv32-linux-gcc (GCC) 9.3.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> << WARNING: modpost: vmlinux.o(__ex_table+0xed0): Section mismatch in reference from the (unknown reference) (unknown) to the variable .debug_str:.LASF1611
>>> WARNING: modpost: vmlinux.o(__ex_table+0xf40): Section mismatch in reference from the (unknown reference) (unknown) to the variable .debug_str:.LASF1482
> FATAL: modpost: The relocation at __ex_table+0xf40 references
> section ".debug_str" which is not executable, IOW
> it is not possible for the kernel to fault
> at that address. Something is seriously wrong
> and should be fixed.
>
Hmm, is this really a build problem introduce by the patch ? I'll try to
investigate a bit more but looks to me that there is another problem and a false
positive.
Thanks,
Enric
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-27 8:59 ` [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling Enric Balletbo i Serra
2020-08-27 19:34 ` kernel test robot
@ 2020-08-31 9:32 ` Bilal Wasim
2020-08-31 9:45 ` Enric Balletbo i Serra
2020-09-02 0:13 ` Bilal Wasim
2020-09-15 13:12 ` Neil Armstrong
3 siblings, 1 reply; 10+ messages in thread
From: Bilal Wasim @ 2020-08-31 9:32 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Neil Armstrong, dri-devel
Hi Enric,
On Thu, 27 Aug 2020 10:59:11 +0200
Enric Balletbo i Serra <enric.balletbo@collabora.com> 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>
> ---
>
> Changes in v3:
> - Make poweron/poweroff and pre_enable/post_disable reverse one to
> each other (Sam Ravnborg)
>
> Changes in v2:
> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam
> Ravnborg)
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 68
> ++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 10
> deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c index
> 9f7b7a9c53c5..7bd0affa057a 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) {
> @@ -152,10 +155,6 @@ static void ps8640_pre_enable(struct drm_bridge
> *bridge) goto err_regulators_disable;
> }
>
> - ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
> - if (ret)
> - goto err_regulators_disable;
> -
> /* Switch access edp panel's edid through i2c */
> ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
> I2C_BYPASS_EN);
> @@ -164,6 +163,8 @@ static void ps8640_pre_enable(struct drm_bridge
> *bridge) goto err_regulators_disable;
> }
>
> + ps_bridge->powered = true;
> +
> return;
>
> err_regulators_disable:
> @@ -171,12 +172,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 +185,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, ENABLE);
> + 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 +272,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.
> + */
> + drm_bridge_chain_pre_enable(bridge);
Are we sure that pre_enable is always good enough to get the EDID? I
know that we only have support for ps8640 on the MT8173 SoC which works
only with pre_enable, but I think a more scalable solution would be to
call drm_bridge_chain_pre_enable / drm_bridge_chain_enable here, and
drm_bridge_chain_post_disable / drm_bridge_chain_disable when disabling
the chain. If this is not a concern and we are sure that pre_enable
will always work (especially on newer boards), then please ignore my
comment.
Other than this, everything looks fine.
>
> - 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)
> + drm_bridge_chain_post_disable(bridge);
> +
> + return edid;
> }
>
> static const struct drm_bridge_funcs ps8640_bridge_funcs = {
-Bilal
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-31 9:32 ` Bilal Wasim
@ 2020-08-31 9:45 ` Enric Balletbo i Serra
0 siblings, 0 replies; 10+ messages in thread
From: Enric Balletbo i Serra @ 2020-08-31 9:45 UTC (permalink / raw)
To: Bilal Wasim
Cc: linux-kernel, Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Neil Armstrong, dri-devel
Hi Bilal,
On 31/8/20 11:32, Bilal Wasim wrote:
>
> Hi Enric,
>
> On Thu, 27 Aug 2020 10:59:11 +0200
> Enric Balletbo i Serra <enric.balletbo@collabora.com> 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>
>> ---
>>
>> Changes in v3:
>> - Make poweron/poweroff and pre_enable/post_disable reverse one to
>> each other (Sam Ravnborg)
>>
>> Changes in v2:
>> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam
>> Ravnborg)
>>
>> drivers/gpu/drm/bridge/parade-ps8640.c | 68
>> ++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 10
>> deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
>> b/drivers/gpu/drm/bridge/parade-ps8640.c index
>> 9f7b7a9c53c5..7bd0affa057a 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) {
>> @@ -152,10 +155,6 @@ static void ps8640_pre_enable(struct drm_bridge
>> *bridge) goto err_regulators_disable;
>> }
>>
>> - ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
>> - if (ret)
>> - goto err_regulators_disable;
>> -
>> /* Switch access edp panel's edid through i2c */
>> ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
>> I2C_BYPASS_EN);
>> @@ -164,6 +163,8 @@ static void ps8640_pre_enable(struct drm_bridge
>> *bridge) goto err_regulators_disable;
>> }
>>
>> + ps_bridge->powered = true;
>> +
>> return;
>>
>> err_regulators_disable:
>> @@ -171,12 +172,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 +185,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, ENABLE);
>> + 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 +272,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.
>> + */
>> + drm_bridge_chain_pre_enable(bridge);
>
> Are we sure that pre_enable is always good enough to get the EDID? I
> know that we only have support for ps8640 on the MT8173 SoC which works
> only with pre_enable, but I think a more scalable solution would be to
> call drm_bridge_chain_pre_enable / drm_bridge_chain_enable here, and
> drm_bridge_chain_post_disable / drm_bridge_chain_disable when disabling
> the chain. If this is not a concern and we are sure that pre_enable
> will always work (especially on newer boards), then please ignore my
> comment.
>
Not a drm_bridge API expert, and sometimes I am confused about it, but, from
what I know, I'm pretty sure that we _don't_ want to call drm_bridge_chain_enable().
The call drm_bridge_chain_pre_enable() will end with calling
drm_panel_prepare(), which, as per documentation:
* Calling this function will enable power and deassert any reset signals to
* the panel. After this has completed it is possible to communicate with any
* integrated circuitry via a command bus.
Calling drm_bridge_chain_enable() too will end calling drm_panel_enable(), and,
as per documentation:
* Calling this function will cause the panel display drivers to be turned on
* and the backlight to be enabled. Content will be visible on screen after
* this call completes
Which can trigger a screen flickering and enable more things that what we want.
Reading the EDID should be possible just after a call of drm_bridge_prepare().
Enable the panel should not be required.
Cheers,
Enric
> Other than this, everything looks fine.
>
>>
>> - 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)
>> + drm_bridge_chain_post_disable(bridge);
>> +
>> + return edid;
>> }
>>
>> static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>
> -Bilal
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-27 8:59 ` [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling Enric Balletbo i Serra
2020-08-27 19:34 ` kernel test robot
2020-08-31 9:32 ` Bilal Wasim
@ 2020-09-02 0:13 ` Bilal Wasim
2020-09-15 13:12 ` Neil Armstrong
3 siblings, 0 replies; 10+ messages in thread
From: Bilal Wasim @ 2020-09-02 0:13 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, Neil Armstrong, dri-devel
Hi Enric,
On Thu, 27 Aug 2020 10:59:11 +0200
Enric Balletbo i Serra <enric.balletbo@collabora.com> 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>
> ---
>
> Changes in v3:
> - Make poweron/poweroff and pre_enable/post_disable reverse one to
> each other (Sam Ravnborg)
>
> Changes in v2:
> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam
> Ravnborg)
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 68
> ++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 10
> deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c index
> 9f7b7a9c53c5..7bd0affa057a 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) {
> @@ -152,10 +155,6 @@ static void ps8640_pre_enable(struct drm_bridge
> *bridge) goto err_regulators_disable;
> }
>
> - ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
> - if (ret)
> - goto err_regulators_disable;
> -
> /* Switch access edp panel's edid through i2c */
> ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
> I2C_BYPASS_EN);
> @@ -164,6 +163,8 @@ static void ps8640_pre_enable(struct drm_bridge
> *bridge) goto err_regulators_disable;
> }
>
> + ps_bridge->powered = true;
> +
> return;
>
> err_regulators_disable:
> @@ -171,12 +172,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 +185,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, ENABLE);
> + 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 +272,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.
> + */
> + drm_bridge_chain_pre_enable(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)
> + drm_bridge_chain_post_disable(bridge);
> +
> + return edid;
> }
>
> static const struct drm_bridge_funcs ps8640_bridge_funcs = {
I was able to apply this patch successfully to drm-misc-next / master -
For Master, I had to apply your patches which have already made it to
drm-misc-next.
I was able to build / test successfully with Elm Chromebook. It seems
that the kernel bot has reported a false positive.
Tested-by: Bilal Wasim <bwasim.lkml@gmail.com>
-Bilal
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling
2020-08-27 8:59 ` [PATCH v3 1/1] drm/bridge: ps8640: Rework power state handling Enric Balletbo i Serra
` (2 preceding siblings ...)
2020-09-02 0:13 ` Bilal Wasim
@ 2020-09-15 13:12 ` Neil Armstrong
3 siblings, 0 replies; 10+ messages in thread
From: Neil Armstrong @ 2020-09-15 13:12 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kernel
Cc: Collabora Kernel ML, matthias.bgg, drinkcat, hsinyi,
laurent.pinchart, sam, Andrzej Hajda, Daniel Vetter, David Airlie,
Jernej Skrabec, Jonas Karlman, dri-devel
On 27/08/2020 10:59, 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>
> ---
>
> Changes in v3:
> - Make poweron/poweroff and pre_enable/post_disable reverse one to each other (Sam Ravnborg)
>
> Changes in v2:
> - Use drm_bridge_chain_pre_enable/post_disable() helpers (Sam Ravnborg)
>
> drivers/gpu/drm/bridge/parade-ps8640.c | 68 ++++++++++++++++++++++----
> 1 file changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 9f7b7a9c53c5..7bd0affa057a 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) {
> @@ -152,10 +155,6 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> goto err_regulators_disable;
> }
>
> - ret = ps8640_bridge_vdo_control(ps_bridge, ENABLE);
> - if (ret)
> - goto err_regulators_disable;
> -
> /* Switch access edp panel's edid through i2c */
> ret = i2c_smbus_write_byte_data(client, PAGE2_I2C_BYPASS,
> I2C_BYPASS_EN);
> @@ -164,6 +163,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
> goto err_regulators_disable;
> }
>
> + ps_bridge->powered = true;
> +
> return;
>
> err_regulators_disable:
> @@ -171,12 +172,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 +185,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, ENABLE);
> + 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 +272,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.
> + */
> + drm_bridge_chain_pre_enable(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)
> + drm_bridge_chain_post_disable(bridge);
> +
> + return edid;
> }
>
> static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
^ permalink raw reply [flat|nested] 10+ messages in thread