On Mon, Jul 16, 2018 at 04:22:15PM -0700, Rodrigo Vivi wrote:
> On Mon, Jul 16, 2018 at 04:05:52PM -0700, Paulo Zanoni wrote:
> > Em Seg, 2018-07-16 às 15:47 -0700, Rodrigo Vivi escreveu:
> > > On Fri, Jul 13, 2018 at 03:57:45PM -0700, Paulo Zanoni wrote:
> > > > Em Sex, 2018-07-13 às 14:08 -0700, Rodrigo Vivi escreveu:
> > > > > On Wed, Jul 11, 2018 at 02:59:02PM -0700, Paulo Zanoni wrote:
> > > > > > Use the hardcoded tables provided by our spec.
> > > > > >
> > > > > > v2:
> > > > > > - SSC stays disabled.
> > > > > > - Use intel_port_is_tc().
> > > > > >
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 22
> > > > > > +++++++++++++++++++++-
> > > > > > 1 file changed, 21 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > index b51ad2917dbe..7e5e6eb5dfe2 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > > > > @@ -2452,6 +2452,16 @@ static const struct skl_wrpll_params
> > > > > > icl_dp_combo_pll_19_2MHz_values[] = {
> > > > > > .pdiv = 0x1 /* 2 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0},
> > > > > > };
> > > > > >
> > > > > > +static const struct skl_wrpll_params icl_tbt_pll_24MHz_values
> > > > > > = {
> > > > > > + .dco_integer = 0x151, .dco_fraction = 0x4000,
> > > > > > + .pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct skl_wrpll_params
> > > > > > icl_tbt_pll_19_2MHz_values =
> > > > > > {
> > > > > > + .dco_integer = 0x1A5, .dco_fraction = 0x7000,
> > > > > > + .pdiv = 0x4 /* 5 */, .kdiv = 1, .qdiv_mode = 0,
> > > > > > .qdiv_ratio = 0,
> > > > >
> > > > > in other words, in a cleaner view:
> > > > >
> > > > > s/qdiv_ratio = 0/qdiv_ratio = 1/g #both tables
> > > >
> > > > From the qdiv ratio bit:
> > > >
> > > > "This field specifies the Q divider ratio. This field is only used
> > > > when
> > > > Qdiv Mode is set to Enable to get a divider value other than 1.".
> > > >
> > > > So having 0 or 1 shouldn't matter since qdiv_mode is zero.
> > > >
> > > > On the other hand, if you look at the Combo PLL table, when
> > > > qdiv_mode=0
> > > > it explicitly tells us to use qdiv_ratio=0, opposite to what you're
> > > > asking.
> > > >
> > > > Then the example below and also the DSI example later both keep
> > > > saying
> > > > to use qdiv_mode=0 + qdiv_ratio=0.
> > > >
> > > > Now the TBT PLL example later asks for qdiv_mode=0 qdiv_ratio=1,
> > > > which
> > > > is what you're asking here, but it's the only the only thing that
> > > > asks
> > > > for that pattern in that way, which makes me believe that either
> > > > it's
> > > > wrong (unlikely) or it simply doesn't matter (as written in the bit
> > > > definition).
> > >
> > > (just adding a note about what we already discussed in pvt last
> > > friday)
> > >
> > > On the TBL section there are 3 places they add qdiv = 1, so spec is
> > > clearly
> > > non-sense because at same time it "indicates" that for qmode=0 qdiv
> > > is always 1
> > > it asks driver to set, because qmode for TBL is always 0 anyways.
> > >
> > > So in my view or bspec is just completely wrong or it is stating that
> > > it is
> > > driver's responsibility to set it to 1.
> > >
> > > Anyways since you already filed the bug against the bspec we could
> > > just move
> > > to either way for now... and fix later...
> > >
> > > However one thing still bugs me... the mismatch of code and spec
> > > table. Even
> > > if spec tells it doesn't matter but they don't update the value
> > > there.
> >
> > As I explained in person, I don't think there is a mismatch of code and
> > table.
>
> ok, so this is the part that we disagree mostly ;)
> I look to spec and there is qdiv = 1 I look to the code and I see qdiv = 0
> for my average developer brain this is a mismatch.
No disagreements anymore. Spec was updated so code matches that now.
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Thanks for filing the spec bug.
>
> > When the spec says "qdiv" it means the high-level qdiv value,
> > which is set through the combination of qdiv_mode and qdiv_ratio.
> >
> > So the TBT table says "qdiv=1", which you can achieve by setting
> > qdiv_mode=0 qdiv_ratio=whatever.
> >
> > The Combo table is more explicit that it lists both qdiv_mode and
> > qdiv_ratio to be set, and it also lists the real "qdiv" as a number in
> > parens on the qdiv_ratio column.
>
> I know, but I shouldn't have to look to other table than the tbl one to
> check tbt code and arrive to conclusions... Actually you pointed me that
> table at first hand because I was looking to the Combo one instead of the TBT :P
>
> >
> > The only thing that really "mismatches" is that in the examples, when
> > qdiv is 1, in three places they say we need qdiv_mode=0 qdiv_ratio=0
>
> on Combo section... on TBT section itself they all mention qdiv=1.
>
> > and in the TBT example they say to use qdiv_mode=0 and qdiv_ratio=1.
> > But that perceived "mismatch" shouldn't matter since when qdiv_mode=0
> > qdiv_ratio can be anything. Still inconsistent, but IMHO irrelevant.
> >
> > >
> > > I can see more devs in the future getting confused because qdiv=1 is
> > > part of
> > > spec's table and code's table mismatch setting it to 0.
> >
> > I think your confusion is that you don't see qdiv as a thing that is
> > achieved through qdiv_mode and qdiv_ratio.
> >
> > >
> > > >
> > > > >
> > > > > with that:
> > > > >
> > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > > >
> > > > > > +};
> > > > > > +
> > > > > > static bool icl_calc_dp_combo_pll(struct drm_i915_private
> > > > > > *dev_priv, int clock,
> > > > > > struct skl_wrpll_params
> > > > > > *pll_params)
> > > > > > {
> > > > > > @@ -2494,6 +2504,14 @@ static bool icl_calc_dp_combo_pll(struct
> > > > > > drm_i915_private *dev_priv, int clock,
> > > > > > return true;
> > > > > > }
> > > > > >
> > > > > > +static bool icl_calc_tbt_pll(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > int clock,
> > > > > > + struct skl_wrpll_params
> > > > > > *pll_params)
> > > > > > +{
> > > > > > + *pll_params = dev_priv->cdclk.hw.ref == 24000 ?
> > > > > > + icl_tbt_pll_24MHz_values :
> > > > > > icl_tbt_pll_19_2MHz_values;
> > > > > > + return true;
> > > > > > +}
> > > > > > +
> > > > > > static bool icl_calc_dpll_state(struct intel_crtc_state
> > > > > > *crtc_state,
> > > > > > struct intel_encoder *encoder,
> > > > > > int
> > > > > > clock,
> > > > > > struct intel_dpll_hw_state
> > > > > > *pll_state)
> > > > > > @@ -2503,7 +2521,9 @@ static bool icl_calc_dpll_state(struct
> > > > > > intel_crtc_state *crtc_state,
> > > > > > struct skl_wrpll_params pll_params = { 0 };
> > > > > > bool ret;
> > > > > >
> > > > > > - if (intel_crtc_has_type(crtc_state,
> > > > > > INTEL_OUTPUT_HDMI))
> > > > > > + if (intel_port_is_tc(dev_priv, encoder->port))
> > > > > > + ret = icl_calc_tbt_pll(dev_priv, clock,
> > > > > > &pll_params);
> > > > > > + else if (intel_crtc_has_type(crtc_state,
> > > > > > INTEL_OUTPUT_HDMI))
> > > > > > ret = cnl_ddi_calculate_wrpll(clock, dev_priv,
> > > > > > &pll_params);
> > > > > > else
> > > > > > ret = icl_calc_dp_combo_pll(dev_priv, clock,
> > > > > > &pll_params);
> > > > > > --
> > > > > > 2.14.4
> > > > > >
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
