From: Rodrigo Vivi <rodrigo.vivi@intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Yet another if/else sort of newer to older platforms.
Date: Fri, 1 Mar 2019 09:24:18 -0800 [thread overview]
Message-ID: <20190301172418.GA8482@intel.com> (raw)
In-Reply-To: <20190228231939.6a4ty5ojyfncrs5n@ldmartin-desk.amr.corp.intel.com>
On Thu, Feb 28, 2019 at 03:19:39PM -0800, Lucas De Marchi wrote:
> On Mon, Feb 25, 2019 at 11:11:30AM -0800, Rodrigo Vivi wrote:
> > No functional change. Just a reorg to match the preferred
> > behavior.
> >
> > When rebasing internal branch on top of latest sort I noticed
> > few more cases that needs to get reordered.
> >
> > Let's do in a bundle this time and hoping there's no other
> > missing places.
> >
> > v2: Check for HSW/BDW ULT before generic IS_HASWELL or
> > IS_BROADWELL or it doesn't work as pointed by Ville.
> > But also ULT came afterwards anyway.
> >
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 20 ++++----
> > drivers/gpu/drm/i915/i915_perf.c | 50 +++++++++---------
> > drivers/gpu/drm/i915/intel_cdclk.c | 38 +++++++-------
> > drivers/gpu/drm/i915/intel_workarounds.c | 64 ++++++++++++------------
> > 4 files changed, 86 insertions(+), 86 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index c6354f6cdbdb..ed48aac1487d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -219,20 +219,20 @@ intel_virt_detect_pch(const struct drm_i915_private *dev_priv)
> > * make an educated guess as to which PCH is really there.
> > */
> >
> > - if (IS_GEN(dev_priv, 5))
> > - id = INTEL_PCH_IBX_DEVICE_ID_TYPE;
> > - else if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > - id = INTEL_PCH_CPT_DEVICE_ID_TYPE;
> > + if (IS_ICELAKE(dev_priv))
> > + id = INTEL_PCH_ICP_DEVICE_ID_TYPE;
> > + else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
>
> if you want to be strict about the order, then this should be:
accepted. It seems the most used one and makes sense
to keep CFL near the gen9 ones although cfl came after cnl
chronologically
>
> else if (IS_CANNONLAKE(dev_priv) || IS_COFFELAKE(dev_priv))
>
> > + id = INTEL_PCH_CNP_DEVICE_ID_TYPE;
> > + else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
>
> ditto
accepted.
>
> > + id = INTEL_PCH_SPT_DEVICE_ID_TYPE;
> > else if (IS_HSW_ULT(dev_priv) || IS_BDW_ULT(dev_priv))
> > id = INTEL_PCH_LPT_LP_DEVICE_ID_TYPE;
> > else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > id = INTEL_PCH_LPT_DEVICE_ID_TYPE;
> > - else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> > - id = INTEL_PCH_SPT_DEVICE_ID_TYPE;
> > - else if (IS_COFFEELAKE(dev_priv) || IS_CANNONLAKE(dev_priv))
> > - id = INTEL_PCH_CNP_DEVICE_ID_TYPE;
> > - else if (IS_ICELAKE(dev_priv))
> > - id = INTEL_PCH_ICP_DEVICE_ID_TYPE;
> > + else if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > + id = INTEL_PCH_CPT_DEVICE_ID_TYPE;
> > + else if (IS_GEN(dev_priv, 5))
> > + id = INTEL_PCH_IBX_DEVICE_ID_TYPE;
> >
> > if (id)
> > DRM_DEBUG_KMS("Assuming PCH ID %04x\n", id);
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 9ebf99f3d8d3..72a9a35b40e2 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2881,12 +2881,24 @@ void i915_perf_register(struct drm_i915_private *dev_priv)
> >
> > sysfs_attr_init(&dev_priv->perf.oa.test_config.sysfs_metric_id.attr);
> >
> > - if (IS_HASWELL(dev_priv)) {
> > - i915_perf_load_test_config_hsw(dev_priv);
> > - } else if (IS_BROADWELL(dev_priv)) {
> > - i915_perf_load_test_config_bdw(dev_priv);
> > - } else if (IS_CHERRYVIEW(dev_priv)) {
> > - i915_perf_load_test_config_chv(dev_priv);
> > + if (IS_ICELAKE(dev_priv)) {
> > + i915_perf_load_test_config_icl(dev_priv);
> > + } else if (IS_CANNONLAKE(dev_priv)) {
> > + i915_perf_load_test_config_cnl(dev_priv);
> > + } else if (IS_COFFEELAKE(dev_priv)) {
> > + if (IS_CFL_GT2(dev_priv))
> > + i915_perf_load_test_config_cflgt2(dev_priv);
> > + if (IS_CFL_GT3(dev_priv))
> > + i915_perf_load_test_config_cflgt3(dev_priv);
> > + } else if (IS_GEMINILAKE(dev_priv)) {
> > + i915_perf_load_test_config_glk(dev_priv);
> > + } else if (IS_KABYLAKE(dev_priv)) {
> > + if (IS_KBL_GT2(dev_priv))
> > + i915_perf_load_test_config_kblgt2(dev_priv);
> > + else if (IS_KBL_GT3(dev_priv))
> > + i915_perf_load_test_config_kblgt3(dev_priv);
> > + } else if (IS_BROXTON(dev_priv)) {
> > + i915_perf_load_test_config_bxt(dev_priv);
> > } else if (IS_SKYLAKE(dev_priv)) {
> > if (IS_SKL_GT2(dev_priv))
> > i915_perf_load_test_config_sklgt2(dev_priv);
> > @@ -2894,25 +2906,13 @@ void i915_perf_register(struct drm_i915_private *dev_priv)
> > i915_perf_load_test_config_sklgt3(dev_priv);
> > else if (IS_SKL_GT4(dev_priv))
> > i915_perf_load_test_config_sklgt4(dev_priv);
> > - } else if (IS_BROXTON(dev_priv)) {
> > - i915_perf_load_test_config_bxt(dev_priv);
> > - } else if (IS_KABYLAKE(dev_priv)) {
> > - if (IS_KBL_GT2(dev_priv))
> > - i915_perf_load_test_config_kblgt2(dev_priv);
> > - else if (IS_KBL_GT3(dev_priv))
> > - i915_perf_load_test_config_kblgt3(dev_priv);
> > - } else if (IS_GEMINILAKE(dev_priv)) {
> > - i915_perf_load_test_config_glk(dev_priv);
> > - } else if (IS_COFFEELAKE(dev_priv)) {
> > - if (IS_CFL_GT2(dev_priv))
> > - i915_perf_load_test_config_cflgt2(dev_priv);
> > - if (IS_CFL_GT3(dev_priv))
> > - i915_perf_load_test_config_cflgt3(dev_priv);
> > - } else if (IS_CANNONLAKE(dev_priv)) {
> > - i915_perf_load_test_config_cnl(dev_priv);
> > - } else if (IS_ICELAKE(dev_priv)) {
> > - i915_perf_load_test_config_icl(dev_priv);
> > - }
> > + } else if (IS_CHERRYVIEW(dev_priv)) {
> > + i915_perf_load_test_config_chv(dev_priv);
> > + } else if (IS_BROADWELL(dev_priv)) {
> > + i915_perf_load_test_config_bdw(dev_priv);
> > + } else if (IS_HASWELL(dev_priv)) {
> > + i915_perf_load_test_config_hsw(dev_priv);
> > +}
> >
> > if (dev_priv->perf.oa.test_config.id == 0)
> > goto sysfs_error;
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 26e01a8465af..5d266538036d 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -2744,18 +2744,13 @@ void intel_update_rawclk(struct drm_i915_private *dev_priv)
> > */
> > void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> > {
> > - if (IS_CHERRYVIEW(dev_priv)) {
> > - dev_priv->display.set_cdclk = chv_set_cdclk;
> > - dev_priv->display.modeset_calc_cdclk =
> > - vlv_modeset_calc_cdclk;
> > - } else if (IS_VALLEYVIEW(dev_priv)) {
> > - dev_priv->display.set_cdclk = vlv_set_cdclk;
> > - dev_priv->display.modeset_calc_cdclk =
> > - vlv_modeset_calc_cdclk;
> > - } else if (IS_BROADWELL(dev_priv)) {
> > - dev_priv->display.set_cdclk = bdw_set_cdclk;
> > + if (IS_ICELAKE(dev_priv)) {
> > + dev_priv->display.set_cdclk = icl_set_cdclk;
> > + dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk;
> > + } else if (IS_CANNONLAKE(dev_priv)) {
> > + dev_priv->display.set_cdclk = cnl_set_cdclk;
> > dev_priv->display.modeset_calc_cdclk =
> > - bdw_modeset_calc_cdclk;
> > + cnl_modeset_calc_cdclk;
> > } else if (IS_GEN9_LP(dev_priv)) {
> > dev_priv->display.set_cdclk = bxt_set_cdclk;
> > dev_priv->display.modeset_calc_cdclk =
> > @@ -2764,23 +2759,28 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> > dev_priv->display.set_cdclk = skl_set_cdclk;
> > dev_priv->display.modeset_calc_cdclk =
> > skl_modeset_calc_cdclk;
> > - } else if (IS_CANNONLAKE(dev_priv)) {
> > - dev_priv->display.set_cdclk = cnl_set_cdclk;
> > + } else if (IS_BROADWELL(dev_priv)) {
> > + dev_priv->display.set_cdclk = bdw_set_cdclk;
> > dev_priv->display.modeset_calc_cdclk =
> > - cnl_modeset_calc_cdclk;
> > - } else if (IS_ICELAKE(dev_priv)) {
> > - dev_priv->display.set_cdclk = icl_set_cdclk;
> > - dev_priv->display.modeset_calc_cdclk = icl_modeset_calc_cdclk;
> > + bdw_modeset_calc_cdclk;
> > + } else if (IS_CHERRYVIEW(dev_priv)) {
> > + dev_priv->display.set_cdclk = chv_set_cdclk;
> > + dev_priv->display.modeset_calc_cdclk =
> > + vlv_modeset_calc_cdclk;
> > + } else if (IS_VALLEYVIEW(dev_priv)) {
> > + dev_priv->display.set_cdclk = vlv_set_cdclk;
> > + dev_priv->display.modeset_calc_cdclk =
> > + vlv_modeset_calc_cdclk;
> > }
> >
> > if (IS_ICELAKE(dev_priv))
> > dev_priv->display.get_cdclk = icl_get_cdclk;
> > else if (IS_CANNONLAKE(dev_priv))
> > dev_priv->display.get_cdclk = cnl_get_cdclk;
> > - else if (IS_GEN9_BC(dev_priv))
> > - dev_priv->display.get_cdclk = skl_get_cdclk;
> > else if (IS_GEN9_LP(dev_priv))
> > dev_priv->display.get_cdclk = bxt_get_cdclk;
> > + else if (IS_GEN9_BC(dev_priv))
> > + dev_priv->display.get_cdclk = skl_get_cdclk;
>
> no reason to change these...
LP came after BC chronologically and I changed just to match
the above group.
>
> > else if (IS_BROADWELL(dev_priv))
> > dev_priv->display.get_cdclk = bdw_get_cdclk;
> > else if (IS_HASWELL(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> > index 743cf5b00155..3e8138b66191 100644
> > --- a/drivers/gpu/drm/i915/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> > @@ -862,26 +862,26 @@ icl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
> > static void
> > gt_init_workarounds(struct drm_i915_private *i915, struct i915_wa_list *wal)
> > {
> > - if (INTEL_GEN(i915) < 8)
> > + if (IS_ICELAKE(i915))
> > + icl_gt_workarounds_init(i915, wal);
> > + else if (IS_CANNONLAKE(i915))
> > + cnl_gt_workarounds_init(i915, wal);
> > + else if (IS_COFFEELAKE(i915))
> > + cfl_gt_workarounds_init(i915, wal);
> > + else if (IS_GEMINILAKE(i915))
> > + glk_gt_workarounds_init(i915, wal);
> > + else if (IS_KABYLAKE(i915))
> > + kbl_gt_workarounds_init(i915, wal);
> > + else if (IS_BROXTON(i915))
> > + bxt_gt_workarounds_init(i915, wal);
> > + else if (IS_SKYLAKE(i915))
> > + skl_gt_workarounds_init(i915, wal);
> > + else if (IS_CHERRYVIEW(i915))
> > return;
> > else if (IS_BROADWELL(i915))
> > return;
> > - else if (IS_CHERRYVIEW(i915))
> > + else if (INTEL_GEN(i915) < 8)
> > return;
> > - else if (IS_SKYLAKE(i915))
> > - skl_gt_workarounds_init(i915, wal);
> > - else if (IS_BROXTON(i915))
> > - bxt_gt_workarounds_init(i915, wal);
> > - else if (IS_KABYLAKE(i915))
> > - kbl_gt_workarounds_init(i915, wal);
> > - else if (IS_GEMINILAKE(i915))
> > - glk_gt_workarounds_init(i915, wal);
> > - else if (IS_COFFEELAKE(i915))
> > - cfl_gt_workarounds_init(i915, wal);
> > - else if (IS_CANNONLAKE(i915))
> > - cnl_gt_workarounds_init(i915, wal);
> > - else if (IS_ICELAKE(i915))
> > - icl_gt_workarounds_init(i915, wal);
> > else
> > MISSING_CASE(INTEL_GEN(i915));
> > }
> > @@ -1063,26 +1063,26 @@ void intel_engine_init_whitelist(struct intel_engine_cs *engine)
> >
> > wa_init_start(w, "whitelist");
> >
> > - if (INTEL_GEN(i915) < 8)
> > + if (IS_ICELAKE(i915))
> > + icl_whitelist_build(w);
> > + else if (IS_CANNONLAKE(i915))
> > + cnl_whitelist_build(w);
> > + else if (IS_COFFEELAKE(i915))
> > + cfl_whitelist_build(w);
> > + else if (IS_GEMINILAKE(i915))
> > + glk_whitelist_build(w);
> > + else if (IS_KABYLAKE(i915))
> > + kbl_whitelist_build(w);
> > + else if (IS_BROXTON(i915))
> > + bxt_whitelist_build(w);
> > + else if (IS_SKYLAKE(i915))
> > + skl_whitelist_build(w);
> > + else if (IS_CHERRYVIEW(i915))
> > return;
> > else if (IS_BROADWELL(i915))
> > return;
> > - else if (IS_CHERRYVIEW(i915))
> > + else if (INTEL_GEN(i915) < 8)
>
> make this INTEL_GEN(i915) <= 8) and remove IS_CHERRYVIEW() above?
accepted.
removing chv and bdw ;)
and I found another similar spot and removing as well...
> The meaning would be that "BROADWELL is the only gen8 that has this, the
> others and below => do nothing."
>
>
> I went through each of the if/else chains and did my best to manually
> verify them: I didn't find any bug. Comments above are basically
> nitpicks. Your call if you are actually going to change them.
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
Thanks,
Rodrigo.
>
>
> Lucas De Marchi
>
> > return;
> > - else if (IS_SKYLAKE(i915))
> > - skl_whitelist_build(w);
> > - else if (IS_BROXTON(i915))
> > - bxt_whitelist_build(w);
> > - else if (IS_KABYLAKE(i915))
> > - kbl_whitelist_build(w);
> > - else if (IS_GEMINILAKE(i915))
> > - glk_whitelist_build(w);
> > - else if (IS_COFFEELAKE(i915))
> > - cfl_whitelist_build(w);
> > - else if (IS_CANNONLAKE(i915))
> > - cnl_whitelist_build(w);
> > - else if (IS_ICELAKE(i915))
> > - icl_whitelist_build(w);
> > else
> > MISSING_CASE(INTEL_GEN(i915));
> >
> > --
> > 2.20.1
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-03-01 17:24 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-22 22:11 [PATCH] drm/i915: Yet another if/else sort of newer to older platforms Rodrigo Vivi
2019-02-22 22:59 ` ✗ Fi.CI.BAT: failure for " Patchwork
2019-02-25 16:32 ` [PATCH] " Ville Syrjälä
2019-02-25 19:08 ` Rodrigo Vivi
2019-02-25 19:11 ` Rodrigo Vivi
2019-02-28 23:19 ` Lucas De Marchi
2019-03-01 17:24 ` Rodrigo Vivi [this message]
2019-03-01 17:27 ` Rodrigo Vivi
2019-02-25 19:58 ` ✓ Fi.CI.BAT: success for drm/i915: Yet another if/else sort of newer to older platforms. (rev2) Patchwork
2019-02-26 4:44 ` ✓ Fi.CI.IGT: " Patchwork
2019-03-01 18:46 ` ✓ Fi.CI.BAT: success for drm/i915: Yet another if/else sort of newer to older platforms. (rev3) Patchwork
2019-03-02 1:09 ` ✓ Fi.CI.IGT: " Patchwork
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=20190301172418.GA8482@intel.com \
--to=rodrigo.vivi@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@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.