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

From: Hans de Goede <hdegoede@redhat.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Hans de Goede <j.w.r.degoede@gmail.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/i915/intel_dsi: Read back pclk set by GOP and use that as pclk
Date: Mon, 9 Jul 2018 19:43:28 +0200	[thread overview]
Message-ID: <baca0ce6-2fb1-67ce-9fff-58e0d24d1604@redhat.com> (raw)
In-Reply-To: <20180709173747.GC28253@intel.com>

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
>>>
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx