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

[Lucas De Marchi]

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.

>
> 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.

>
> 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).


>         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.


thanks
Lucas De Marchi