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

From: Sean Paul <seanpaul@chromium.org>
To: Andrzej Hajda <a.hajda@samsung.com>
Cc: zain wang <wzz@rock-chips.com>, Lin Huang <hl@rock-chips.com>,
	Tomeu Vizoso <tomeu.vizoso@collabora.com>,
	Douglas Anderson <dianders@chromium.org>,
	dri-devel@lists.freedesktop.org,
	linux-rockchip@lists.infradead.org,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Yakir Yang <ykk@rock-chips.com>
Subject: Re: [PATCH 12/41] drm/bridge: analogix_dp: add fast link train for eDP
Date: Tue, 21 Mar 2017 16:37:28 -0400	[thread overview]
Message-ID: <20170321203728.GL19389@art_vandelay> (raw)
In-Reply-To: <3a7cc255-084f-7367-e9de-831a9bc96191@samsung.com>

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@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@rock-chips.com>
> > Signed-off-by: Sean Paul <seanpaul@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.

<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.

> 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;
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel