On Thu, 06 Oct 2016, "Vivi, Rodrigo" <rodrigo.vivi@xxxxxxxxx> wrote:
> On Tue, 2016-10-04 at 10:05 +0300, Jani Nikula wrote:
>> On Tue, 04 Oct 2016, "Vivi, Rodrigo" <rodrigo.vivi@xxxxxxxxx> wrote:
>> > On Mon, 2016-10-03 at 13:50 +0300, Jani Nikula wrote:
>> >> On Fri, 30 Sep 2016, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
>> >> > According to spec: "KBL re-uses SKL values, except where
>> >> > specific KBL values are listed."
>> >> >
>> >> > And recently spec has changed adding different table for Display Port only.
>> >> > But for all SKUs (H,S,U,Y) we have slightly different values.
>> >> >
>> >> > Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> >> > Cc: Arthur Runyan <arthur.j.runyan@xxxxxxxxx>
>> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> >> > ---
>> >> > drivers/gpu/drm/i915/intel_ddi.c | 88 +++++++++++++++++++++++++++++++++++-----
>> >> > 1 file changed, 78 insertions(+), 10 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> >> > index 018964b..1573360 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> >> > @@ -167,8 +167,47 @@ static const struct ddi_buf_trans skl_y_ddi_translations_dp[] = {
>> >> > { 0x80005012, 0x000000C0, 0x3 },
>> >> > };
>> >> >
>> >> > +/* Kabylake H and S */
>> >> > +static const struct ddi_buf_trans kbl_ddi_translations_dp[] = {
>> >> > + { 0x00002016, 0x000000A0, 0x0 },
>> >> > + { 0x00005012, 0x0000009B, 0x0 },
>> >> > + { 0x00007011, 0x00000088, 0x0 },
>> >> > + { 0x80009010, 0x000000C0, 0x1 },
>> >> > + { 0x00002016, 0x0000009B, 0x0 },
>> >> > + { 0x00005012, 0x00000088, 0x0 },
>> >> > + { 0x80007011, 0x000000C0, 0x1 },
>> >> > + { 0x00002016, 0x0000009F, 0x0 },
>> >> > + { 0x80005012, 0x000000C0, 0x1 },
>> >> > +};
>> >> > +
>> >> > +/* Kabylake U */
>> >> > +static const struct ddi_buf_trans kbl_u_ddi_translations_dp[] = {
>> >> > + { 0x0000201B, 0x000000A1, 0x0 },
>> >> > + { 0x00005012, 0x00000088, 0x0 },
>> >> > + { 0x80007011, 0x000000CD, 0x3 },
>> >> > + { 0x80009010, 0x000000C0, 0x3 },
>> >> > + { 0x0000201B, 0x0000009D, 0x0 },
>> >> > + { 0x80005012, 0x000000C0, 0x3 },
>> >> > + { 0x80007011, 0x000000C0, 0x3 },
>> >> > + { 0x00002016, 0x0000004F, 0x0 },
>> >> > + { 0x80005012, 0x000000C0, 0x3 },
>> >> > +};
>> >> > +
>> >> > +/* Kabylake Y */
>> >> > +static const struct ddi_buf_trans kbl_y_ddi_translations_dp[] = {
>> >> > + { 0x00001017, 0x000000A1, 0x0 },
>> >> > + { 0x00005012, 0x00000088, 0x0 },
>> >> > + { 0x80007011, 0x000000CD, 0x3 },
>> >> > + { 0x8000800F, 0x000000C0, 0x3 },
>> >> > + { 0x00001017, 0x0000009D, 0x0 },
>> >> > + { 0x80005012, 0x000000C0, 0x3 },
>> >> > + { 0x80007011, 0x000000C0, 0x3 },
>> >> > + { 0x00001017, 0x0000004C, 0x0 },
>> >> > + { 0x80005012, 0x000000C0, 0x3 },
>> >> > +};
>> >> > +
>> >> > /*
>> >> > - * Skylake H and S
>> >> > + * Skylake/Kabylake H and S
>> >> > * eDP 1.4 low vswing translation parameters
>> >> > */
>> >> > static const struct ddi_buf_trans skl_ddi_translations_edp[] = {
>> >> > @@ -185,7 +224,7 @@ static const struct ddi_buf_trans skl_ddi_translations_edp[] = {
>> >> > };
>> >> >
>> >> > /*
>> >> > - * Skylake U
>> >> > + * Skylake/Kabylake U
>> >> > * eDP 1.4 low vswing translation parameters
>> >> > */
>> >> > static const struct ddi_buf_trans skl_u_ddi_translations_edp[] = {
>> >> > @@ -202,7 +241,7 @@ static const struct ddi_buf_trans skl_u_ddi_translations_edp[] = {
>> >> > };
>> >> >
>> >> > /*
>> >> > - * Skylake Y
>> >> > + * Skylake/Kabylake Y
>> >> > * eDP 1.4 low vswing translation parameters
>> >> > */
>> >> > static const struct ddi_buf_trans skl_y_ddi_translations_edp[] = {
>> >> > @@ -218,7 +257,7 @@ static const struct ddi_buf_trans skl_y_ddi_translations_edp[] = {
>> >> > { 0x00000018, 0x0000008A, 0x0 },
>> >> > };
>> >> >
>> >> > -/* Skylake U, H and S */
>> >> > +/* Skylake/Kabylake U, H and S */
>> >> > static const struct ddi_buf_trans skl_ddi_translations_hdmi[] = {
>> >> > { 0x00000018, 0x000000AC, 0x0 },
>> >> > { 0x00005012, 0x0000009D, 0x0 },
>> >> > @@ -233,7 +272,7 @@ static const struct ddi_buf_trans skl_ddi_translations_hdmi[] = {
>> >> > { 0x80000018, 0x000000C0, 0x1 },
>> >> > };
>> >> >
>> >> > -/* Skylake Y */
>> >> > +/* Skylake/Kabylake Y */
>> >> > static const struct ddi_buf_trans skl_y_ddi_translations_hdmi[] = {
>> >> > { 0x00000018, 0x000000A1, 0x0 },
>> >> > { 0x00005012, 0x000000DF, 0x0 },
>> >> > @@ -334,10 +373,10 @@ bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > static const struct ddi_buf_trans *
>> >> > skl_get_buf_trans_dp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > {
>> >> > - if (IS_SKL_ULX(dev_priv) || IS_KBL_ULX(dev_priv)) {
>> >> > + if (IS_SKL_ULX(dev_priv)) {
>> >> > *n_entries = ARRAY_SIZE(skl_y_ddi_translations_dp);
>> >> > return skl_y_ddi_translations_dp;
>> >> > - } else if (IS_SKL_ULT(dev_priv) || IS_KBL_ULT(dev_priv)) {
>> >> > + } else if (IS_SKL_ULT(dev_priv)) {
>> >> > *n_entries = ARRAY_SIZE(skl_u_ddi_translations_dp);
>> >> > return skl_u_ddi_translations_dp;
>> >> > } else {
>> >> > @@ -347,6 +386,21 @@ skl_get_buf_trans_dp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > }
>> >> >
>> >> > static const struct ddi_buf_trans *
>> >> > +kbl_get_buf_trans_dp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > +{
>> >> > + if (IS_KBL_ULX(dev_priv)) {
>> >> > + *n_entries = ARRAY_SIZE(kbl_y_ddi_translations_dp);
>> >> > + return kbl_y_ddi_translations_dp;
>> >> > + } else if (IS_KBL_ULT(dev_priv)) {
>> >> > + *n_entries = ARRAY_SIZE(kbl_u_ddi_translations_dp);
>> >> > + return kbl_u_ddi_translations_dp;
>> >> > + } else {
>> >> > + *n_entries = ARRAY_SIZE(kbl_ddi_translations_dp);
>> >> > + return kbl_ddi_translations_dp;
>> >> > + }
>> >> > +}
>> >> > +
>> >> > +static const struct ddi_buf_trans *
>> >> > skl_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > {
>> >> > if (dev_priv->vbt.edp.low_vswing) {
>> >> > @@ -362,7 +416,10 @@ skl_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>> >> > }
>> >> > }
>> >> >
>> >> > - return skl_get_buf_trans_dp(dev_priv, n_entries);
>> >> > + if (dev_priv->vbt.edp.low_vswing)
>> >> > + return kbl_get_buf_trans_dp(dev_priv, n_entries);
>> >> > + else
>> >> > + return skl_get_buf_trans_dp(dev_priv, n_entries);
>> >>
>> >> This seems really confusing.
>> >
>> > I agree, but I would like to avoid duplicating the identical tables...
>> > those by spec are explicitly same table...
>> >
>> > another option without duplicating the tables is duplicating the
>> > function like below... what I believe it is still confusing with all
>> > those skl and kbl inside this function...
>> >
>> > what do you prefer?
>>
>> The thing that bugs me here is that you may return klb_something on
>> skl. We reuse plenty of older gen stuff for newer gens, but this being
>> the other way round is the confusing part.
>
>
> No, it is not the other way around.
>
> we have many cases of:
> void old_function() {
> IS_NEWER()
> do_something();
> /*old colde*/
> }
It's okay if you explicitly do it like this, but in your patch the
condition is edp low_vswing, not platform.
BR,
Jani.
>
> Other way around for me would be
> a kbl function with IS_SKYLAKE inside...
>
> But I understand what bugs you. I'm also not convinced this option here
> is the best one, but this is the minimal duplication of code.
>
> The second option is to have to different functions almost identical one
> for skl and one for kbl and the third one duplicating even the tables.
>
> We just need to decide what level of duplication would be ok.
>
>> So just call it
>> skl_something, and reuse in kbl?
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > static const struct ddi_buf_trans
>> > *
>> > kbl_get_buf_trans_edp(struct drm_i915_private *dev_priv, int
>> > *n_entries)
>> > {
>> > if (dev_priv->vbt.edp.low_vswing)
>> > {
>> > if (IS_KBL_ULX(dev_priv))
>> > {
>> > *n_entries =
>> > ARRAY_SIZE(skl_y_ddi_translations_edp);
>> > return
>> > skl_y_ddi_translations_edp;
>> > } else if (IS_KBL_ULT(dev_priv))
>> > {
>> > *n_entries =
>> > ARRAY_SIZE(skl_u_ddi_translations_edp);
>> > return
>> > skl_u_ddi_translations_edp;
>> > } else
>> > {
>> > *n_entries =
>> > ARRAY_SIZE(skl_ddi_translations_edp);
>> > return
>> > skl_ddi_translations_edp;
>> > }
>> > }
>> > return kbl_get_buf_trans_dp(dev_priv,
>> > n_entries);
>> > }
>> >
>> >
>> >
>> >
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >> > }
>> >> >
>> >> > static const struct ddi_buf_trans *
>> >> > @@ -430,7 +487,13 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder)
>> >> > if (IS_BROXTON(dev_priv))
>> >> > return;
>> >> >
>> >> > - if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
>> >> > + if (IS_KABYLAKE(dev_priv)) {
>> >> > + ddi_translations_fdi = NULL;
>> >> > + ddi_translations_dp =
>> >> > + kbl_get_buf_trans_dp(dev_priv, &n_dp_entries);
>> >> > + ddi_translations_edp =
>> >> > + skl_get_buf_trans_edp(dev_priv, &n_edp_entries);
>> >> > + } else if (IS_SKYLAKE(dev_priv)) {
>> >> > ddi_translations_fdi = NULL;
>> >> > ddi_translations_dp =
>> >> > skl_get_buf_trans_dp(dev_priv, &n_dp_entries);
>> >> > @@ -1437,7 +1500,12 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, u32 level)
>> >> > if (dp_iboost) {
>> >> > iboost = dp_iboost;
>> >> > } else {
>> >> > - ddi_translations = skl_get_buf_trans_dp(dev_priv, &n_entries);
>> >> > + if (IS_KABYLAKE(dev_priv))
>> >> > + ddi_translations = kbl_get_buf_trans_dp(dev_priv,
>> >> > + &n_entries);
>> >> > + else
>> >> > + ddi_translations = skl_get_buf_trans_dp(dev_priv,
>> >> > + &n_entries);
>> >> > iboost = ddi_translations[level].i_boost;
>> >> > }
>> >> > } else if (type == INTEL_OUTPUT_EDP) {
>> >>
>> >
>>
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
