[Intel-gfx] [PATCH] drm/i915/vlv: Add cdclk workaround for DSI

[Rodrigo Vivi]

On Tue, Dec 19, 2017 at 08:38:10PM +0000, Hans de Goede wrote:
> Hi,
> 
> On 19-12-17 20:42, Rodrigo Vivi wrote:
> > 
> > On Tue, Dec 19, 2017 at 07:21:13PM +0000, Hans de Goede wrote:
> > > At least on the Chuwi Vi8 (non pro/plus) the LCD panel will show an image
> > > shifted aprox. 20% to the left (with wraparound) and sometimes also wrong
> > > colors, showing that the panel controller is starting with sampling the
> > > datastream somewhere mid-line. This happens after the first blanking and
> > > re-init of the panel.
> > > 
> > > After looking at drm.debug output I noticed that initially we inherit the
> > > cdclk of 333333 KHz set by the GOP, but after the re-init we picked 266667
> > > KHz, which turns out to be the cause of this problem, a quick hack to hard
> > > code the cdclk to 333333 KHz makes the problem go away.
> > > 
> > > I've tested this on various Bay Trail devices:
> > > -Chuwi Vi8: 800x1280, 333333 at boot, requires 333333 to work properly
> > > -GP-electronic T701: 1024x600, 333333 at boot, works fine with 333333
> > > -PEAQ C1010: 1920x1200, 333333 at boot, works fine with 333333
> > > -PoV mobii-wintab-800w: 800x1280, 333333 at boot, works fine with 333333
> > > -Asus Transformer-T100TA: 1368x768, 320000 at boot, works fine with 320000
> > 
> > and lower clock shift the image in all of them?
> 
> No, just on the Chuwi Vi8, the tests was to make sure the higher clock
> does not create problems elsewhere and I was curious what clock the GOP
> was using on other devices. Also hence the "requires 333333 to work properly"
> vs "works fine with 333333" in the list.
> 
> > > So it seems that the GOP is always using what vlv_calc_cdclk calls
> > > freq_320 as cdclk clock. Also note the comment in vlv_calc_cdclk about
> > > it avoiding 200 Mhz as clock because that causes issues in some cases.
> > > 
> > > This commit extends the "do not use 200 Mhz" workaround with an extra
> > > check to require atleast 320000 KHz (avoiding 266667 KHz) when a DSI
> > > panel is active.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 9c5ceb98d48f..cfb3f7fb3e1c 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1923,6 +1923,14 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > >   	if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > >   		min_cdclk = max(2 * 96000, min_cdclk);
> > > +	/*
> > > +	 * On Valleyview the GOP always uses 320000/333333 KHz if DSI is used,
> > > +	 * using a lower clock causes causes sync issues with some panels.
> > > +	 */
> > 
> > Afaik (afai-have-noticed) GOP always use the higher freq and higher rates
> > that panels support with no care about saving any power. So I'm not sure if
> > that should be used as any sort of reference for us.
> > 
> > Note I'm not telling to not add the workaround itself...
> 
> So you're suggesting to change the comment into something like this
> instead ?  :
> 
> /*
>  * On Valleyview some DSI panels loose (v|h)sync when the clock is lower
>  * then 320000KHz.
>  */

Yeap... something like that so we don't start counting GOP as the reference in the future ;)

> 
> > 
> > > +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DSI) &&
> > > +	    IS_VALLEYVIEW(dev_priv))
> > > +		min_cdclk = max(320000, min_cdclk);
> > > +
> > >   	if (min_cdclk > dev_priv->max_cdclk_freq) {
> > >   		DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > >   			      min_cdclk, dev_priv->max_cdclk_freq);
> > > -- 
> > > 2.14.3
> > > 
> 
> Regards,
> 
> Hans