| Message ID | 20171219192113.12841-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, I forgot to add a coverletter, anyways what I wanted to put in the coverletter and not in the commit message is a link to a picture of the problem this fixes: https://fedorapeople.org/~jwrdegoede/IMG_20171217_195637.jpg Note the screen is actually a portrait screen, so the picture is the right way up. Beside the obvious left shift with wrap-around of the picture, also all the green in there is supposed to be blue. Another less clear picture is this one: https://fedorapeople.org/~jwrdegoede/IMG_20171217_195507.jpg Again with wrong colors. ### While working on this I also noticed that the pixelclock as the driver gets it from the VBT is not the same as the one the GOP uses, on the tablet in question the VBT says 77000 KHz and the GOP uses (according to the initial readback) 78125 KHz, on another tablet I noticed the VBT saying 78125 KHz, where as the GOP was using 68xxx KHz which came to a refresh-rate of around 60 Hz, where as the VBT value is 69 Hz IIRC. Neither of these cause any actual issue (fixing the pixelclock to match the GOP programmed value does not fix the issue, where as using the GOP cdclk of 333333KHz does). Regards, Hans On 19-12-17 20:21, 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 > > 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@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. > + */ > + 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); >
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? > > 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@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... > + 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 >
On Tue, Dec 19, 2017 at 08:31:07PM +0100, Hans de Goede wrote: > Hi, > > I forgot to add a coverletter, anyways what I wanted to put in the > coverletter and not in the commit message is a link to a picture of the > problem this fixes: > > https://fedorapeople.org/~jwrdegoede/IMG_20171217_195637.jpg > > Note the screen is actually a portrait screen, so the picture is the > right way up. Beside the obvious left shift with wrap-around of the > picture, also all the green in there is supposed to be blue. > > Another less clear picture is this one: > > https://fedorapeople.org/~jwrdegoede/IMG_20171217_195507.jpg > > Again with wrong colors. > > ### > > While working on this I also noticed that the pixelclock as > the driver gets it from the VBT is not the same as the one the > GOP uses, on the tablet in question the VBT says 77000 KHz > and the GOP uses (according to the initial readback) 78125 KHz, > on another tablet I noticed the VBT saying 78125 KHz, where > as the GOP was using 68xxx KHz which came to a refresh-rate > of around 60 Hz, where as the VBT value is 69 Hz IIRC. > > Neither of these cause any actual issue (fixing the pixelclock > to match the GOP programmed value does not fix the issue, where > as using the GOP cdclk of 333333KHz does). https://patchwork.freedesktop.org/patch/127189/ I suppose it's unlike that would help here since it's just about the overlap in dual link mode, but there could be other fail in the DSI clock code.
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@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. */ > >> + 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
Hi, On 19-12-17 20:48, Ville Syrjälä wrote: > On Tue, Dec 19, 2017 at 08:31:07PM +0100, Hans de Goede wrote: >> Hi, >> >> I forgot to add a coverletter, anyways what I wanted to put in the >> coverletter and not in the commit message is a link to a picture of the >> problem this fixes: >> >> https://fedorapeople.org/~jwrdegoede/IMG_20171217_195637.jpg >> >> Note the screen is actually a portrait screen, so the picture is the >> right way up. Beside the obvious left shift with wrap-around of the >> picture, also all the green in there is supposed to be blue. >> >> Another less clear picture is this one: >> >> https://fedorapeople.org/~jwrdegoede/IMG_20171217_195507.jpg >> >> Again with wrong colors. >> >> ### >> >> While working on this I also noticed that the pixelclock as >> the driver gets it from the VBT is not the same as the one the >> GOP uses, on the tablet in question the VBT says 77000 KHz >> and the GOP uses (according to the initial readback) 78125 KHz, >> on another tablet I noticed the VBT saying 78125 KHz, where >> as the GOP was using 68xxx KHz which came to a refresh-rate >> of around 60 Hz, where as the VBT value is 69 Hz IIRC. >> >> Neither of these cause any actual issue (fixing the pixelclock >> to match the GOP programmed value does not fix the issue, where >> as using the GOP cdclk of 333333KHz does). > > https://patchwork.freedesktop.org/patch/127189/ > > I suppose it's unlike that would help here since it's just about the > overlap in dual link mode, but there could be other fail in the DSI > clock code. Yes that is is not applicable to the Vi8 where I'm seeing the 77000 KHz used by GOP vs 78125 KHz used by i915. Looking at intel_dsi_vbt_init the pclk on the boards where I'm seeing an inconsistency is just taken directly from mode->clock, which in turn is simply dvo_timing->clock * 10. Again: these different pixel-clocks do not seem to be an issue, I just noticed this too while looking at the problem best seen here: https://fedorapeople.org/~jwrdegoede/IMG_20171217_195637.jpg Regards, Hans
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@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
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. + */ + 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);
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 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@redhat.com> --- drivers/gpu/drm/i915/intel_cdclk.c | 8 ++++++++ 1 file changed, 8 insertions(+)