[PATCH 12/41] drm/bridge: analogix_dp: add fast link train for eDP

[Andrzej Hajda]

On 21.03.2017 21:37, Sean Paul wrote:
> On Thu, Mar 16, 2017 at 03:14:28PM +0100, Andrzej Hajda wrote:
>> On 10.03.2017 05:32, Sean Paul wrote:
>>> From: zain wang <wzz at rock-chips.com>
>>>
>>> We would meet a short black screen when exit PSR with the full link
>>> training, In this case, we should use fast link train instead of full
>>> link training.
>>>
>>> Signed-off-by: zain wang <wzz at rock-chips.com>
>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>> ---
>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 142 ++++++++++++++++-----
>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   3 +
>>>  2 files changed, 114 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 64d94a34874d..5bc151b0995b 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -10,17 +10,18 @@
>>>  * option) any later version.
>>>  */
>>>  
>>> -#include <linux/module.h>
>>> -#include <linux/platform_device.h>
>>> -#include <linux/err.h>
>>>  #include <linux/clk.h>
>>> -#include <linux/io.h>
>>> +#include <linux/component.h>
>>> +#include <linux/err.h>
>>> +#include <linux/gpio.h>
>>>  #include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/module.h>
>>>  #include <linux/of.h>
>>>  #include <linux/of_gpio.h>
>>> -#include <linux/gpio.h>
>>> -#include <linux/component.h>
>>>  #include <linux/phy/phy.h>
>>> +#include <linux/platform_device.h>
>>>  
>>>  #include <drm/drmP.h>
>>>  #include <drm/drm_atomic_helper.h>
>>> @@ -35,6 +36,8 @@
>>>  
>>>  #define to_dp(nm)	container_of(nm, struct analogix_dp_device, nm)
>>>  
>>> +static const bool verify_fast_training;
>>> +
>> Quite odd debug sentinel, I am not sure if it is good practice to use
>> such construct.
> IIRC, they originally had a #define and then used #ifdef below to gate the
> verification. My concern with that was the code would rot if it wasn't compiled.

It just looks unusual, I am not strongly against current version.

>
> <snip>
>
>>> @@ -853,10 +933,10 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
>>>  			DRM_ERROR("failed to disable the panel\n");
>>>  	}
>>>  
>>> -	ret = analogix_dp_set_link_train(dp, dp->video_info.max_lane_count,
>>> -					 dp->video_info.max_link_rate);
>>> +	ret = readx_poll_timeout(analogix_dp_train_link, dp, ret, !ret, 100,
>>> +				 DP_TIMEOUT_TRAINING_US * 5);
>>>  	if (ret) {
>>> -		dev_err(dp->dev, "unable to do link train\n");
>>> +		dev_err(dp->dev, "unable to do link train, ret=%d\n", ret);
>> And here we have ", ret=%d\n". Maybe it would be good to make it
>> consistent across whole driver.
> Meh. I don't think it's worth bikeshedding over commas in error messages. I'm
> just happy there are error checks/messages.

But if there will be next iteration it is not big deal to fix it then :)

Regards
Andrzej

>
>
>> Another issue with consistency in DRM_DEV_ERROR vs dev_err.
> Yeah, that's annoying.
>
> Sean
>
>> Regards
>> Andrzej
>>
>>>  		return;
>>>  	}
>>>  
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> index e135a42cb19e..920607d7eb3e 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> @@ -20,6 +20,8 @@
>>>  #define MAX_CR_LOOP 5
>>>  #define MAX_EQ_LOOP 5
>>>  
>>> +/* Training takes 22ms if AUX channel comm fails. Use this as retry interval */
>>> +#define DP_TIMEOUT_TRAINING_US			22000
>>>  #define DP_TIMEOUT_PSR_LOOP_MS			300
>>>  
>>>  /* DP_MAX_LANE_COUNT */
>>> @@ -171,6 +173,7 @@ struct analogix_dp_device {
>>>  	int			hpd_gpio;
>>>  	bool                    force_hpd;
>>>  	bool			psr_enable;
>>> +	bool			fast_train_support;
>>>  
>>>  	struct mutex		panel_lock;
>>>  	bool			panel_is_modeset;