Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel