[Intel-gfx] [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Lucas De Marchi]

On Tue, Nov 14, 2017 at 5:10 AM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Mon, Nov 13, 2017 at 01:47:26PM -0800, Lucas De Marchi wrote:
>> Hi Ville,
>>
>> On Thu, Nov 9, 2017 at 8:58 AM, Ville Syrjälä
>> <ville.syrjala at linux.intel.com> wrote:
>> > On Thu, Nov 09, 2017 at 08:02:40AM -0800, Lucas De Marchi wrote:
>> >> On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
>> >> <ville.syrjala at linux.intel.com> wrote:
>> >> > On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
>> >> >> Wa Display #1183 was recently added to workaround
>> >> >> "Failures when enabling DPLL0 with eDP link rate 2.16
>> >> >> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
>> >> >> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
>> >> >>  enabling or in previous enabling."
>> >> >>
>> >> >> This Workaround was designed to minimize the impact only
>> >> >> to save the bad case with that link rates. But HW engineers
>> >> >> indicated that it should be safe to apply broadly. Although
>> >> >> they were expecting the DPLL0 link rate to be unchanged on
>> >> >> runtime.
>> >> >>
>> >> >> We need to cover 2 cases: when we are in fact enabling DPLL0
>> >> >> and when we are just changing the frequency. The workaround
>> >> >> for those cases are similar but different enough to have them
>> >> >> done in different places.
>> >> >>
>> >> >> This is based on previous patch by Rodrigo Vivi with suggestions
>> >> >> from Ville Syrjälä.
>> >> >
>> >> > Still doesn't look like what I suggested.
>> >>
>> >> I agree with your suggestion of moving stuff to skl_set_cdclk() to
>> >> cover the case in which
>> >> vco isn't changing. However see the paragraph I added above on why I
>> >> need to do it
>> >> differently. In short: the sequence on the WA for enabling and
>> >> updating cdclck are different,
>> >> with some code duplication unfortunately. I don't see you covering
>> >> that case in your
>> >> suggestion. Have I missed anything?
>> >
>> > Even if we follow the spec literally I think we can do it all
>> > in skl_set_cdclk(). I think the following should dtrt:
>>
>> There are some subtle differences wrt to the initialize and update
>> sequences according to the WA
>> that I'd like to clarify.
>>
>> >
>> > pcu start
>> >
>> > if (...)
>> >         disable_dpll0()
>> >
>> > cdclk_sel = real
>>
>> We should only do this if we are enabling, but not when updating. In
>> the latter case
>> cdclk_sel should only be touched after setting divmux to 1.
>
> Seems like a pointless distinction to me. We'll be doing the 0->real
> toggle anyway while divmux_override==1. But if we want to be pedantic,
> then we could of course just skip this if the DPLL is already enabled.
>
>>
>> >
>> > if (need_wa)
>> >         divmux=1
>>
>> Reading the WA to the letter, in the enabling case this should happen between
>> DPLL_CTRL1 and LCPLL1_CTL are written.  Here you are moving it to happen before
>> the write to DPLL_CTRL1.
>
> I assume that until the DPLL is enabled the settings in DPLL_CTRL1
> don't actually matter.
>
>>
>> >
>> > if (...)
>> >         enable_dpll0()
>> >
>> > if (need_wa) {
>> >         cdclk_sel = 0
>> >         cdclk_sel = real
>>
>> When updating we should set both freq_sel and and freq_decimal. When
>> enabling, only freq_sel (but I guess
>> it doesn't matter since we set this same register above).
>
> I think the implication is just that the "decimal" frequency doesn't
> matter until something actually starts to use cdclk. The safe bet
> would be to always program it to match the frequency select.
>
>>
>>
>> >         divmux=0
>> > }
>>
>> With this sequence you would actually not change the frequency for the
>> cases in which the WA is not
>> required. AFAIU from previous version of this patch it's ok to always
>> follow the WA path so we wouldn't
>> have a "need_wa". Is that ok?  I can come up with a patch that shares
>> more code, but I don't think your
>> approach is following the spec literally.
>
> Yeah, the exact conditions for need_wa seem a bit unlcear to me since it
> says "... used in this enabling or in previous enabling". I'm not sure
> if it's referring to the DPLL or CDCLK frequency or both. Maybe safer to
> just always follow the w/a sequence.


Ok. I thought it would be better to follow the exact steps from the WA
as I actually
couldn't reproduce the bug. I implemented what you said and tested
both approaches
to check it's not regressing.  I will submit a new version with your
comments addressed.

thanks
Lucas De Marchi