| Message ID | 20180619201827.4257-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote: > On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) > different frequency then the pclk which we've calculated. > > This commit makes the DSI code read-back the pclk set by the GOP and > if that is within a reasonable margin of the calculated pclk, uses > that instead. > > This fixes the first modeset being a full modeset instead of a > fast modeset on systems where the GOP pclk is different. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c > index 4d6ffa7b3e7b..d4cc6099012c 100644 > --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c > +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c > @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > u32 mul; > u16 burst_mode_ratio; > enum port port; > + enum pipe pipe; > > DRM_DEBUG_KMS("\n"); > > @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > } else > burst_mode_ratio = 100; > > + /* > + * On BYT / CRC the GOP sometimes picks a slightly different pclk, > + * read back the GOP configured pclk and prefer it over ours. > + */ > + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && > + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { > + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); > + > + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); > + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) > + pclk = gop; > + } Is the gop acually picking a different clock that what we pick in the end, or is it just that the value in the vbt doesn't quite match what we (and the gop) end up using on account of limitations of the pll? For that particular problem I think I had patches long ago to go through the pll computation during init so that we basically fix up the slightly bogus clock from the vbt. Any kind of hack that involves reading out the hardware state should go into something like intel_sanitize_encoder(). Actually by that time we have already read out the hw state, so it shouldn't require any modifications to the existing dsi code itself. > + > intel_dsi->burst_mode_ratio = burst_mode_ratio; > intel_dsi->pclk = pclk; > > -- > 2.17.1
Hi, On 07/06/2018 04:16 PM, Ville Syrjälä wrote: > On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote: >> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) >> different frequency then the pclk which we've calculated. >> >> This commit makes the DSI code read-back the pclk set by the GOP and >> if that is within a reasonable margin of the calculated pclk, uses >> that instead. >> >> This fixes the first modeset being a full modeset instead of a >> fast modeset on systems where the GOP pclk is different. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> index 4d6ffa7b3e7b..d4cc6099012c 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c >> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c >> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> u32 mul; >> u16 burst_mode_ratio; >> enum port port; >> + enum pipe pipe; >> >> DRM_DEBUG_KMS("\n"); >> >> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >> } else >> burst_mode_ratio = 100; >> >> + /* >> + * On BYT / CRC the GOP sometimes picks a slightly different pclk, >> + * read back the GOP configured pclk and prefer it over ours. >> + */ >> + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && >> + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { >> + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); >> + >> + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); >> + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) >> + pclk = gop; >> + } > > Is the gop acually picking a different clock that what we pick in the > end, or is it just that the value in the vbt doesn't quite match what we > (and the gop) end up using on account of limitations of the pll? I *think* the GOP is picking a different clock, IIRC (*) it is something like 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it unmodified. With this patch which puts pclk at 90Mhz on the specific tablet I developed this on, the PLL settings calculated by our PLL code end up being exactly the same as the once chosen by the GOP once we have the pclk set to 90MHz. Note I've seen these small (and sometimes somewhat bigger) differences between GOP and VBT pclk on a lot of devices, not just the one tablet I developed it on. Since submitting this I've run this on at least 5 different CHT/BYT devices and it works as advertised so far. > For that particular problem I think I had patches long ago to go through > the pll computation during init so that we basically fix up the slightly > bogus clock from the vbt. We do end up with a slightly different clock then the 87MHhz when going though the PLL calculations, something like 86.33MHz or some such from the top of my head, but the problem is not the pclk not matching the intel_pipe_config_compare() function does not look at it, it looks at dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match if we fixup the VBT clock to be the one confgured by the GOP. > Any kind of hack that involves reading out the hardware state should go > into something like intel_sanitize_encoder(). Actually by that time we > have already read out the hw state, so it shouldn't require any > modifications to the existing dsi code itself. I do not think that intel_sanitize encoder is the right place to do this: * I don't want to modify the read-back state, I want to modify our calculated "new/ideal" state to match the read-back state * I don't want to directly modify our calculated new/ideal state, instead I want to cleanup / sanitize the values we got from the VBT so that the initial-modeset *and* any future modesets will use the GOP pclk. I believe it is important that if we're going to use the GOP pclk we use it for all modesets consistently. * I only want to do this once, at boot when we are sure the mode was set by the GOP and not after suspend/resume when we don't know if the GOP will have touched things or not. * It is DSI specific, whereas sofar intel_sanitize_encoder seems to not contain any output specific code. Regards, Hans > >> + >> intel_dsi->burst_mode_ratio = burst_mode_ratio; >> intel_dsi->pclk = pclk; >> >> -- >> 2.17.1 >
On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote: > Hi, > > On 07/06/2018 04:16 PM, Ville Syrjälä wrote: > > On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote: > > > On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) > > > different frequency then the pclk which we've calculated. > > > > > > This commit makes the DSI code read-back the pclk set by the GOP and > > > if that is within a reasonable margin of the calculated pclk, uses > > > that instead. > > > > > > This fixes the first modeset being a full modeset instead of a > > > fast modeset on systems where the GOP pclk is different. > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c > > > index 4d6ffa7b3e7b..d4cc6099012c 100644 > > > --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c > > > +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c > > > @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > > > u32 mul; > > > u16 burst_mode_ratio; > > > enum port port; > > > + enum pipe pipe; > > > DRM_DEBUG_KMS("\n"); > > > @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > > > } else > > > burst_mode_ratio = 100; > > > + /* > > > + * On BYT / CRC the GOP sometimes picks a slightly different pclk, > > > + * read back the GOP configured pclk and prefer it over ours. > > > + */ > > > + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && > > > + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { > > > + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); > > > + > > > + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); > > > + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) > > > + pclk = gop; > > > + } > > > > Is the gop acually picking a different clock that what we pick in the > > end, or is it just that the value in the vbt doesn't quite match what we > > (and the gop) end up using on account of limitations of the pll? > > I *think* the GOP is picking a different clock, IIRC (*) it is something like > 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it > unmodified. With this patch which puts pclk at 90Mhz on the specific > tablet I developed this on, the PLL settings calculated by our PLL code > end up being exactly the same as the once chosen by the GOP once we have > the pclk set to 90MHz. > > Note I've seen these small (and sometimes somewhat bigger) differences > between GOP and VBT pclk on a lot of devices, not just the one tablet > I developed it on. Since submitting this I've run this on at least > 5 different CHT/BYT devices and it works as advertised so far. It seems GOP is just not respecting VBT and using the whatever it judge the safest option?! Probably not optimal, but I believe we should stay on the safest side as well, if this is the case. > > > For that particular problem I think I had patches long ago to go through > > the pll computation during init so that we basically fix up the slightly > > bogus clock from the vbt. > > We do end up with a slightly different clock then the 87MHhz when going > though the PLL calculations, something like 86.33MHz or some such from > the top of my head, but the problem is not the pclk not matching the > intel_pipe_config_compare() function does not look at it, it looks at > dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match > if we fixup the VBT clock to be the one confgured by the GOP. > > > Any kind of hack that involves reading out the hardware state should go > > into something like intel_sanitize_encoder(). Actually by that time we > > have already read out the hw state, so it shouldn't require any > > modifications to the existing dsi code itself. > > I do not think that intel_sanitize encoder is the right place to do this: > > * I don't want to modify the read-back state, I want to modify our > calculated "new/ideal" state to match the read-back state > * I don't want to directly modify our calculated new/ideal state, > instead I want to cleanup / sanitize the values we got from the VBT > so that the initial-modeset *and* any future modesets will use the > GOP pclk. I believe it is important that if we're going to use the > GOP pclk we use it for all modesets consistently. > * I only want to do this once, at boot when we are sure the mode was > set by the GOP and not after suspend/resume when we don't know if the > GOP will have touched things or not. > * It is DSI specific, whereas sofar intel_sanitize_encoder seems to > not contain any output specific code. > > Regards, > > Hans > > > > > > > > + > > > intel_dsi->burst_mode_ratio = burst_mode_ratio; > > > intel_dsi->pclk = pclk; > > > -- > > > 2.17.1 > > >
Hi, On 07/09/2018 07:37 PM, Rodrigo Vivi wrote: > On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote: >> Hi, >> >> On 07/06/2018 04:16 PM, Ville Syrjälä wrote: >>> On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote: >>>> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) >>>> different frequency then the pclk which we've calculated. >>>> >>>> This commit makes the DSI code read-back the pclk set by the GOP and >>>> if that is within a reasonable margin of the calculated pclk, uses >>>> that instead. >>>> >>>> This fixes the first modeset being a full modeset instead of a >>>> fast modeset on systems where the GOP pclk is different. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c >>>> index 4d6ffa7b3e7b..d4cc6099012c 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c >>>> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c >>>> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >>>> u32 mul; >>>> u16 burst_mode_ratio; >>>> enum port port; >>>> + enum pipe pipe; >>>> DRM_DEBUG_KMS("\n"); >>>> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) >>>> } else >>>> burst_mode_ratio = 100; >>>> + /* >>>> + * On BYT / CRC the GOP sometimes picks a slightly different pclk, >>>> + * read back the GOP configured pclk and prefer it over ours. >>>> + */ >>>> + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && >>>> + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { >>>> + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); >>>> + >>>> + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); >>>> + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) >>>> + pclk = gop; >>>> + } >>> >>> Is the gop acually picking a different clock that what we pick in the >>> end, or is it just that the value in the vbt doesn't quite match what we >>> (and the gop) end up using on account of limitations of the pll? >> >> I *think* the GOP is picking a different clock, IIRC (*) it is something like >> 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it >> unmodified. With this patch which puts pclk at 90Mhz on the specific >> tablet I developed this on, the PLL settings calculated by our PLL code >> end up being exactly the same as the once chosen by the GOP once we have >> the pclk set to 90MHz. >> >> Note I've seen these small (and sometimes somewhat bigger) differences >> between GOP and VBT pclk on a lot of devices, not just the one tablet >> I developed it on. Since submitting this I've run this on at least >> 5 different CHT/BYT devices and it works as advertised so far. > > It seems GOP is just not respecting VBT and using the whatever it judge > the safest option?! Yes, that or it is using some unknown rounding mechanism, to get to a pclk which the pll can represent exactly. If I don't fixup the pclk in the VBT the actually set pclk is somewhat different then the one originally in the VBT as the PLL can not generate an exact match. > Probably not optimal, but I believe we should stay on the safest side as well, > if this is the case. Agreed. Regards, Hans > >> >>> For that particular problem I think I had patches long ago to go through >>> the pll computation during init so that we basically fix up the slightly >>> bogus clock from the vbt. >> >> We do end up with a slightly different clock then the 87MHhz when going >> though the PLL calculations, something like 86.33MHz or some such from >> the top of my head, but the problem is not the pclk not matching the >> intel_pipe_config_compare() function does not look at it, it looks at >> dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match >> if we fixup the VBT clock to be the one confgured by the GOP. >> >>> Any kind of hack that involves reading out the hardware state should go >>> into something like intel_sanitize_encoder(). Actually by that time we >>> have already read out the hw state, so it shouldn't require any >>> modifications to the existing dsi code itself. >> >> I do not think that intel_sanitize encoder is the right place to do this: >> >> * I don't want to modify the read-back state, I want to modify our >> calculated "new/ideal" state to match the read-back state >> * I don't want to directly modify our calculated new/ideal state, >> instead I want to cleanup / sanitize the values we got from the VBT >> so that the initial-modeset *and* any future modesets will use the >> GOP pclk. I believe it is important that if we're going to use the >> GOP pclk we use it for all modesets consistently. >> * I only want to do this once, at boot when we are sure the mode was >> set by the GOP and not after suspend/resume when we don't know if the >> GOP will have touched things or not. >> * It is DSI specific, whereas sofar intel_sanitize_encoder seems to >> not contain any output specific code. >> >> Regards, >> >> Hans >> >> >> >>> >>>> + >>>> intel_dsi->burst_mode_ratio = burst_mode_ratio; >>>> intel_dsi->pclk = pclk; >>>> -- >>>> 2.17.1 >>> >>
On Sat, Jul 07, 2018 at 08:32:16AM +0200, Hans de Goede wrote: > Hi, > > On 07/06/2018 04:16 PM, Ville Syrjälä wrote: > > On Tue, Jun 19, 2018 at 10:18:27PM +0200, Hans de Goede wrote: > >> On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) > >> different frequency then the pclk which we've calculated. > >> > >> This commit makes the DSI code read-back the pclk set by the GOP and > >> if that is within a reasonable margin of the calculated pclk, uses > >> that instead. > >> > >> This fixes the first modeset being a full modeset instead of a > >> fast modeset on systems where the GOP pclk is different. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c > >> index 4d6ffa7b3e7b..d4cc6099012c 100644 > >> --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c > >> +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c > >> @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > >> u32 mul; > >> u16 burst_mode_ratio; > >> enum port port; > >> + enum pipe pipe; > >> > >> DRM_DEBUG_KMS("\n"); > >> > >> @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) > >> } else > >> burst_mode_ratio = 100; > >> > >> + /* > >> + * On BYT / CRC the GOP sometimes picks a slightly different pclk, > >> + * read back the GOP configured pclk and prefer it over ours. > >> + */ > >> + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && > >> + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { > >> + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); > >> + > >> + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); > >> + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) > >> + pclk = gop; > >> + } > > > > Is the gop acually picking a different clock that what we pick in the > > end, or is it just that the value in the vbt doesn't quite match what we > > (and the gop) end up using on account of limitations of the pll? > > I *think* the GOP is picking a different clock, IIRC (*) it is something like > 90Mhz for the GOP and the VBT says 87Mhz (and our calculations leave it > unmodified. With this patch which puts pclk at 90Mhz on the specific > tablet I developed this on, the PLL settings calculated by our PLL code > end up being exactly the same as the once chosen by the GOP once we have > the pclk set to 90MHz. > > Note I've seen these small (and sometimes somewhat bigger) differences > between GOP and VBT pclk on a lot of devices, not just the one tablet > I developed it on. Since submitting this I've run this on at least > 5 different CHT/BYT devices and it works as advertised so far. > > > For that particular problem I think I had patches long ago to go through > > the pll computation during init so that we basically fix up the slightly > > bogus clock from the vbt. > > We do end up with a slightly different clock then the 87MHhz when going > though the PLL calculations, something like 86.33MHz or some such from > the top of my head, but the problem is not the pclk not matching the > intel_pipe_config_compare() function does not look at it, it looks at > dsi_pll.ctrl dsi_pll.div and those don't match, where as they do match > if we fixup the VBT clock to be the one confgured by the GOP. > > > Any kind of hack that involves reading out the hardware state should go > > into something like intel_sanitize_encoder(). Actually by that time we > > have already read out the hw state, so it shouldn't require any > > modifications to the existing dsi code itself. > > I do not think that intel_sanitize encoder is the right place to do this: > > * I don't want to modify the read-back state, I want to modify our > calculated "new/ideal" state to match the read-back state I wasn't suggesting that. What I meant is that you already have the state there to look so you don't have to hack the readout functions to function without a state being around. That said, we do already have intel_encoder_current_mode() which is doing something similar to what you're proposing. So probably should just try to reuse that.
diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c index 4d6ffa7b3e7b..d4cc6099012c 100644 --- a/drivers/gpu/drm/i915/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c @@ -517,6 +517,7 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) u32 mul; u16 burst_mode_ratio; enum port port; + enum pipe pipe; DRM_DEBUG_KMS("\n"); @@ -583,6 +584,19 @@ bool intel_dsi_vbt_init(struct intel_dsi *intel_dsi, u16 panel_id) } else burst_mode_ratio = 100; + /* + * On BYT / CRC the GOP sometimes picks a slightly different pclk, + * read back the GOP configured pclk and prefer it over ours. + */ + if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && + intel_dsi_get_hw_state(&intel_dsi->base, &pipe)) { + u32 gop = intel_dsi_get_pclk(&intel_dsi->base, bpp, NULL); + + DRM_DEBUG_KMS("Calculated pclk %d GOP %d\n", pclk, gop); + if (gop >= (pclk * 9 / 10) && gop <= (pclk * 11 / 10)) + pclk = gop; + } + intel_dsi->burst_mode_ratio = burst_mode_ratio; intel_dsi->pclk = pclk;
On BYT and CHT the GOP sometimes initializes the pclk at a (slightly) different frequency then the pclk which we've calculated. This commit makes the DSI code read-back the pclk set by the GOP and if that is within a reasonable margin of the calculated pclk, uses that instead. This fixes the first modeset being a full modeset instead of a fast modeset on systems where the GOP pclk is different. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/intel_dsi_vbt.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)