On Wed, Nov 28, 2018 at 10:02:22AM +0200, Jani Nikula wrote:
> On Tue, 06 Nov 2018, Lucas De Marchi <lucas.demarchi@xxxxxxxxx> wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> >
> > RANGE makes it longer, but clear. We are also going to add a check for
> > the display part, so make rename to GT.
>
> I also still have my doubts about this patch I'm afraid. I've expressed
> the concern before, but here goes again.
>
> How much is the split of gen to GT gen and display gen going to help us
> in the long run? The only current platform that would benefit from this
> is GLK. However, not all IS_GEMINILAKE() can be replaced with
> IS_DISPLAY_GEN() >= 10 or similar. We also have VLV/CHV display that is
> better represented by HAS_GMCH_DISPLAY() and AFAICT can't usefully be
> replaced by a display gen check.
Fair enough. I believe I have to prove that out with the other series
that I'm planning to put on top.
For this one for now maybe we should just rename IS_GEN() to GEN_RANGE()
And latter on the series introducing the display_gen we could convert
that to GT_GEN_RANGE DISPLAY_GEN_RANGE if that is acceptable.
>
> My main fear is that the split adds a lot of confusion. (Where should I
> use GT gen, where should I use display gen, patches to change between
> one and the other. It's not 100% clear cut.)
Yes, this is a very good point. Specially around some gray areas like PM.
So let's hold this discussion for the next series and first organize
the current gen_range vs is_gen/intel_gen <= etc...
>
> Here too I wonder if we're better off adding more has_feature flags that
> describe what gt or display features a platform has.
This is another thing we should discuss separated of this thread.
I believe that our platform definition and addition as a room for improve
somehow... I just don't have good ideas :/ and I also believe this is
discussion for another series and should not block the progress of
this one.
>
> BR,
> Jani.
>
>
> >
> > Diff generated with:
> >
> > sed 's/IS_GEN(/GT_GEN_RANGE(/g' drivers/gpu/drm/i915/*.{c,h} -i
> >
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/i915_perf.c | 4 ++--
> > drivers/gpu/drm/i915/intel_bios.c | 2 +-
> > drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
> > drivers/gpu/drm/i915/intel_fbc.c | 2 +-
> > drivers/gpu/drm/i915/intel_hangcheck.c | 2 +-
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++----
> > drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
> > 8 files changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8839a34f7648..e4749e93a1f9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2374,7 +2374,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > *
> > * Use GEN_FOREVER for unbound start and or end.
> > */
> > -#define IS_GEN(dev_priv, s, e) \
> > +#define GT_GEN_RANGE(dev_priv, s, e) \
> > (!!((dev_priv)->info.gen_mask & INTEL_GEN_MASK((s), (e))))
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 2c2b63be7a6c..acc3502403b3 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1796,7 +1796,7 @@ static int gen8_enable_metric_set(struct i915_perf_stream *stream)
> > * be read back from automatically triggered reports, as part of the
> > * RPT_ID field.
> > */
> > - if (IS_GEN(dev_priv, 9, 11)) {
> > + if (GT_GEN_RANGE(dev_priv, 9, 11)) {
> > I915_WRITE(GEN8_OA_DEBUG,
> > _MASKED_BIT_ENABLE(GEN9_OA_DEBUG_DISABLE_CLK_RATIO_REPORTS |
> > GEN9_OA_DEBUG_INCLUDE_CLK_RATIO));
> > @@ -3476,7 +3476,7 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> >
> > dev_priv->perf.oa.gen8_valid_ctx_bit = (1<<16);
> > }
> > - } else if (IS_GEN(dev_priv, 10, 11)) {
> > + } else if (GT_GEN_RANGE(dev_priv, 10, 11)) {
> > dev_priv->perf.oa.ops.is_valid_b_counter_reg =
> > gen7_is_valid_b_counter_addr;
> > dev_priv->perf.oa.ops.is_valid_mux_reg =
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 0ad2304457ab..fb918e942e9a 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -446,7 +446,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv, u8 bdb_version)
> > * Only parse SDVO mappings on gens that could have SDVO. This isn't
> > * accurate and doesn't have to be, as long as it's not too strict.
> > */
> > - if (!IS_GEN(dev_priv, 3, 7)) {
> > + if (!GT_GEN_RANGE(dev_priv, 3, 7)) {
> > DRM_DEBUG_KMS("Skipping SDVO device mapping\n");
> > return;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> > index bc147d9e6c92..4f7b654614e9 100644
> > --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> > @@ -1286,7 +1286,7 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
> > &engine->execlists;
> > u64 addr;
> >
> > - if (engine->id == RCS && IS_GEN(dev_priv, 4, 7))
> > + if (engine->id == RCS && GT_GEN_RANGE(dev_priv, 4, 7))
> > drm_printf(m, "\tCCID: 0x%08x\n", I915_READ(CCID));
> > drm_printf(m, "\tRING_START: 0x%08x\n",
> > I915_READ(RING_START(engine->mmio_base)));
> > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> > index 14cbaf4a0e93..9f41779988e5 100644
> > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > @@ -784,7 +784,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
> > * having a Y offset that isn't divisible by 4 causes FIFO underrun
> > * and screen flicker.
> > */
> > - if (IS_GEN(dev_priv, 9, 10) &&
> > + if (GT_GEN_RANGE(dev_priv, 9, 10) &&
> > (fbc->state_cache.plane.adjusted_y & 3)) {
> > fbc->no_fbc_reason = "plane Y offset is misaligned";
> > return false;
> > diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
> > index e26d05a46451..9b15ed1409ae 100644
> > --- a/drivers/gpu/drm/i915/intel_hangcheck.c
> > +++ b/drivers/gpu/drm/i915/intel_hangcheck.c
> > @@ -252,7 +252,7 @@ engine_stuck(struct intel_engine_cs *engine, u64 acthd)
> > return ENGINE_WAIT_KICK;
> > }
> >
> > - if (IS_GEN(dev_priv, 6, 7) && tmp & RING_WAIT_SEMAPHORE) {
> > + if (GT_GEN_RANGE(dev_priv, 6, 7) && tmp & RING_WAIT_SEMAPHORE) {
> > switch (semaphore_passed(engine)) {
> > default:
> > return ENGINE_DEAD;
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index b8a7a014d46d..0e5f49c45298 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -407,7 +407,7 @@ static void intel_ring_setup_status_page(struct intel_engine_cs *engine)
> > POSTING_READ(mmio);
> >
> > /* Flush the TLB for this page */
> > - if (IS_GEN(dev_priv, 6, 7)) {
> > + if (GT_GEN_RANGE(dev_priv, 6, 7)) {
> > i915_reg_t reg = RING_INSTPM(engine->mmio_base);
> >
> > /* ring should be idle before issuing a sync flush*/
> > @@ -629,7 +629,7 @@ static int init_render_ring(struct intel_engine_cs *engine)
> > intel_whitelist_workarounds_apply(engine);
> >
> > /* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */
> > - if (IS_GEN(dev_priv, 4, 6))
> > + if (GT_GEN_RANGE(dev_priv, 4, 6))
> > I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
> >
> > /* We need to disable the AsyncFlip performance optimisations in order
> > @@ -638,7 +638,7 @@ static int init_render_ring(struct intel_engine_cs *engine)
> > *
> > * WaDisableAsyncFlipPerfMode:snb,ivb,hsw,vlv
> > */
> > - if (IS_GEN(dev_priv, 6, 7))
> > + if (GT_GEN_RANGE(dev_priv, 6, 7))
> > I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(ASYNC_FLIP_PERF_DISABLE));
> >
> > /* Required for the hardware to program scanline values for waiting */
> > @@ -663,7 +663,7 @@ static int init_render_ring(struct intel_engine_cs *engine)
> > _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
> > }
> >
> > - if (IS_GEN(dev_priv, 6, 7))
> > + if (GT_GEN_RANGE(dev_priv, 6, 7))
> > I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
> >
> > if (INTEL_GEN(dev_priv) >= 6)
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 9289515108c3..4ffe26cc3c3f 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1567,13 +1567,13 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
> > dev_priv->uncore.pmic_bus_access_nb.notifier_call =
> > i915_pmic_bus_access_notifier;
> >
> > - if (IS_GEN(dev_priv, 2, 4) || intel_vgpu_active(dev_priv)) {
> > + if (GT_GEN_RANGE(dev_priv, 2, 4) || intel_vgpu_active(dev_priv)) {
> > ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen2);
> > ASSIGN_READ_MMIO_VFUNCS(dev_priv, gen2);
> > } else if (IS_GEN5(dev_priv)) {
> > ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen5);
> > ASSIGN_READ_MMIO_VFUNCS(dev_priv, gen5);
> > - } else if (IS_GEN(dev_priv, 6, 7)) {
> > + } else if (GT_GEN_RANGE(dev_priv, 6, 7)) {
> > ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen6);
> >
> > if (IS_VALLEYVIEW(dev_priv)) {
> > @@ -1592,7 +1592,7 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
> > ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, gen8);
> > ASSIGN_READ_MMIO_VFUNCS(dev_priv, gen6);
> > }
> > - } else if (IS_GEN(dev_priv, 9, 10)) {
> > + } else if (GT_GEN_RANGE(dev_priv, 9, 10)) {
> > ASSIGN_FW_DOMAINS_TABLE(__gen9_fw_ranges);
> > ASSIGN_WRITE_MMIO_VFUNCS(dev_priv, fwtable);
> > ASSIGN_READ_MMIO_VFUNCS(dev_priv, fwtable);
> > @@ -2321,7 +2321,7 @@ intel_uncore_forcewake_for_read(struct drm_i915_private *dev_priv,
> > } else if (INTEL_GEN(dev_priv) >= 6) {
> > fw_domains = __gen6_reg_read_fw_domains(offset);
> > } else {
> > - WARN_ON(!IS_GEN(dev_priv, 2, 5));
> > + WARN_ON(!GT_GEN_RANGE(dev_priv, 2, 5));
> > fw_domains = 0;
> > }
> >
> > @@ -2343,10 +2343,10 @@ intel_uncore_forcewake_for_write(struct drm_i915_private *dev_priv,
> > fw_domains = __fwtable_reg_write_fw_domains(offset);
> > } else if (IS_GEN8(dev_priv)) {
> > fw_domains = __gen8_reg_write_fw_domains(offset);
> > - } else if (IS_GEN(dev_priv, 6, 7)) {
> > + } else if (GT_GEN_RANGE(dev_priv, 6, 7)) {
> > fw_domains = FORCEWAKE_RENDER;
> > } else {
> > - WARN_ON(!IS_GEN(dev_priv, 2, 5));
> > + WARN_ON(!GT_GEN_RANGE(dev_priv, 2, 5));
> > fw_domains = 0;
> > }
>
> --
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> 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
