From: Jani Nikula <jani.nikula@linux.intel.com>
To: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"Runyan, Arthur J" <arthur.j.runyan@intel.com>
Subject: Re: [PATCH 2/2] drm/i915: KBL - Recommended buffer translation programming for DisplayPort
Date: Thu, 06 Oct 2016 10:14:18 +0300 [thread overview]
Message-ID: <87bmyyx839.fsf@intel.com> (raw)
In-Reply-To: <1475711990.23853.48.camel@rdvivi-vienna>
On Thu, 06 Oct 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
> On Tue, 2016-10-04 at 10:05 +0300, Jani Nikula wrote:
>> On Tue, 04 Oct 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote:
>> > On Mon, 2016-10-03 at 13:50 +0300, Jani Nikula wrote:
>> >> On Fri, 30 Sep 2016, Rodrigo Vivi <rodrigo.vivi@intel.com> 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@intel.com>
>> >> > Cc: Arthur Runyan <arthur.j.runyan@intel.com>
>> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> >> > ---
>> >> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-10-06 7:14 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-30 18:05 [PATCH 1/2] drm/i915: Move down skl/kbl ddi iboost and n_edp_entires fixup Rodrigo Vivi
2016-09-30 18:05 ` [PATCH 2/2] drm/i915: KBL - Recommended buffer translation programming for DisplayPort Rodrigo Vivi
2016-10-03 10:50 ` Jani Nikula
2016-10-04 0:30 ` Vivi, Rodrigo
2016-10-04 7:05 ` Jani Nikula
2016-10-06 0:00 ` Vivi, Rodrigo
2016-10-06 7:14 ` Jani Nikula [this message]
2016-10-06 17:54 ` [PATCH] " Rodrigo Vivi
2016-10-18 0:28 ` Manasi Navare
2016-10-18 15:57 ` Rodrigo Vivi
2016-10-18 21:42 ` Manasi Navare
2016-09-30 19:19 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Move down skl/kbl ddi iboost and n_edp_entires fixup Patchwork
2016-10-06 18:20 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Move down skl/kbl ddi iboost and n_edp_entires fixup (rev2) Patchwork
2016-10-20 20:28 ` [PATCH 1/2] drm/i915: Move down skl/kbl ddi iboost and n_edp_entires fixup Manasi Navare
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87bmyyx839.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=arthur.j.runyan@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.