| Message ID | 1462544186-34818-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > To be used for more efficient Gen range checking. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ > 3 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 15fcbcece13c..935e381407ba 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { > }) > #define INTEL_INFO(p) (&__I915__(p)->info) > #define INTEL_GEN(p) (INTEL_INFO(p)->gen) > +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) > #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) > #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index d5a7cfec589b..2c3681757aba 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) > > /* FIXME: We lack the proper locking here, so only run this on the > * platforms that need. */ > - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) > + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) > cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); > cache->fb.pixel_format = fb->pixel_format; > cache->fb.stride = fb->pitches[0]; > @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) > _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); > > /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ > - if (IS_GEN7(dev)) > + if (IS_GEN7(dev_priv)) > I915_WRITE(GFX_MODE_GEN7, > _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | > _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); > > - if (IS_GEN6(dev)) { > + if (IS_GEN6(dev_priv)) { This chunk shouldn't be in this patch. Couldn't improve upon INTEL_GEN_RANGE. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 06/05/16 15:33, Chris Wilson wrote: > On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> To be used for more efficient Gen range checking. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_fbc.c | 2 +- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ >> 3 files changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 15fcbcece13c..935e381407ba 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { >> }) >> #define INTEL_INFO(p) (&__I915__(p)->info) >> #define INTEL_GEN(p) (INTEL_INFO(p)->gen) >> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) >> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index d5a7cfec589b..2c3681757aba 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) >> >> /* FIXME: We lack the proper locking here, so only run this on the >> * platforms that need. */ >> - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) >> + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) >> cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); >> cache->fb.pixel_format = fb->pixel_format; >> cache->fb.stride = fb->pitches[0]; >> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) >> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); >> >> /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ >> - if (IS_GEN7(dev)) >> + if (IS_GEN7(dev_priv)) >> I915_WRITE(GFX_MODE_GEN7, >> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | >> _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); >> >> - if (IS_GEN6(dev)) { >> + if (IS_GEN6(dev_priv)) { > > This chunk shouldn't be in this patch. > > Couldn't improve upon INTEL_GEN_RANGE. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris INTEL_GEN_IN_RANGE() ? Perhaps emphasises that we're using INclusive ranges? (whereas the open-coded version might use inclusive or exclusive or semi-inclusive at either end, as in the example above.) .Dave.
On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: > On 06/05/16 15:33, Chris Wilson wrote: >> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> To be used for more efficient Gen range checking. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>> drivers/gpu/drm/i915/intel_fbc.c | 2 +- >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ >>> 3 files changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index 15fcbcece13c..935e381407ba 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { >>> }) >>> #define INTEL_INFO(p) (&__I915__(p)->info) >>> #define INTEL_GEN(p) (INTEL_INFO(p)->gen) >>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) >>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >>> index d5a7cfec589b..2c3681757aba 100644 >>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) >>> >>> /* FIXME: We lack the proper locking here, so only run this on the >>> * platforms that need. */ >>> - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) >>> + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) >>> cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); >>> cache->fb.pixel_format = fb->pixel_format; >>> cache->fb.stride = fb->pitches[0]; >>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) >>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); >>> >>> /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ >>> - if (IS_GEN7(dev)) >>> + if (IS_GEN7(dev_priv)) >>> I915_WRITE(GFX_MODE_GEN7, >>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | >>> _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); >>> >>> - if (IS_GEN6(dev)) { >>> + if (IS_GEN6(dev_priv)) { >> >> This chunk shouldn't be in this patch. >> >> Couldn't improve upon INTEL_GEN_RANGE. >> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> -Chris > > INTEL_GEN_IN_RANGE() ? > > Perhaps emphasises that we're using INclusive ranges? Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and IS_BXT_REVID(). Might as well make this shorter to write. BR, Jani. > > (whereas the open-coded version might use inclusive or exclusive or > semi-inclusive at either end, as in the example above.) > > .Dave. > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 09/05/16 13:32, Jani Nikula wrote: > On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: >> On 06/05/16 15:33, Chris Wilson wrote: >>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> To be used for more efficient Gen range checking. >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>>> drivers/gpu/drm/i915/intel_fbc.c | 2 +- >>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ >>>> 3 files changed, 8 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index 15fcbcece13c..935e381407ba 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { >>>> }) >>>> #define INTEL_INFO(p) (&__I915__(p)->info) >>>> #define INTEL_GEN(p) (INTEL_INFO(p)->gen) >>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) >>>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >>>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >>>> index d5a7cfec589b..2c3681757aba 100644 >>>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) >>>> >>>> /* FIXME: We lack the proper locking here, so only run this on the >>>> * platforms that need. */ >>>> - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) >>>> + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) >>>> cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); >>>> cache->fb.pixel_format = fb->pixel_format; >>>> cache->fb.stride = fb->pitches[0]; >>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) >>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); >>>> >>>> /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ >>>> - if (IS_GEN7(dev)) >>>> + if (IS_GEN7(dev_priv)) >>>> I915_WRITE(GFX_MODE_GEN7, >>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | >>>> _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); >>>> >>>> - if (IS_GEN6(dev)) { >>>> + if (IS_GEN6(dev_priv)) { >>> >>> This chunk shouldn't be in this patch. >>> >>> Couldn't improve upon INTEL_GEN_RANGE. >>> >>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >>> -Chris >> >> INTEL_GEN_IN_RANGE() ? >> >> Perhaps emphasises that we're using INclusive ranges? > > Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and > IS_BXT_REVID(). Might as well make this shorter to write. > > BR, > Jani. We need a notation that will look good (and be efficient) for "before GEN x" and "from GEN y on" (or "up to and including GEN p" and "after GEN q") -- these make up the vast majority of tests on the GEN number. Also, we need real clarity on inclusion/exclusion. since..until notation is perhaps ambiguous - in particular, is "until" included? English usage is not clear here, it's often but not always exclusive! .Dave.
On Mon, 09 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: > On 09/05/16 13:32, Jani Nikula wrote: >> On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: >>> On 06/05/16 15:33, Chris Wilson wrote: >>>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> To be used for more efficient Gen range checking. >>>>> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> --- >>>>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>>>> drivers/gpu/drm/i915/intel_fbc.c | 2 +- >>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ >>>>> 3 files changed, 8 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>>> index 15fcbcece13c..935e381407ba 100644 >>>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { >>>>> }) >>>>> #define INTEL_INFO(p) (&__I915__(p)->info) >>>>> #define INTEL_GEN(p) (INTEL_INFO(p)->gen) >>>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) >>>>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) >>>>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >>>>> index d5a7cfec589b..2c3681757aba 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c >>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c >>>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) >>>>> >>>>> /* FIXME: We lack the proper locking here, so only run this on the >>>>> * platforms that need. */ >>>>> - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) >>>>> + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) >>>>> cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); >>>>> cache->fb.pixel_format = fb->pixel_format; >>>>> cache->fb.stride = fb->pitches[0]; >>>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) >>>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); >>>>> >>>>> /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ >>>>> - if (IS_GEN7(dev)) >>>>> + if (IS_GEN7(dev_priv)) >>>>> I915_WRITE(GFX_MODE_GEN7, >>>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | >>>>> _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); >>>>> >>>>> - if (IS_GEN6(dev)) { >>>>> + if (IS_GEN6(dev_priv)) { >>>> >>>> This chunk shouldn't be in this patch. >>>> >>>> Couldn't improve upon INTEL_GEN_RANGE. >>>> >>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >>>> -Chris >>> >>> INTEL_GEN_IN_RANGE() ? >>> >>> Perhaps emphasises that we're using INclusive ranges? >> >> Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and >> IS_BXT_REVID(). Might as well make this shorter to write. >> >> BR, >> Jani. > > We need a notation that will look good (and be efficient) for "before > GEN x" and "from GEN y on" (or "up to and including GEN p" and "after > GEN q") -- these make up the vast majority of tests on the GEN number. > > Also, we need real clarity on inclusion/exclusion. since..until notation > is perhaps ambiguous - in particular, is "until" included? English usage > is not clear here, it's often but not always exclusive! If you want real clarity, you'll ditch this patch and use if (INTEL_GEN(p) >= 5 && INTEL_GEN(p) <= 7) etc... and be done with it. There's too much churn going on with all the related macros anyway, so I'm not at all convinced about the patches anyway. J. > > .Dave. >
On Mon, May 09, 2016 at 05:40:39PM +0300, Jani Nikula wrote: > On Mon, 09 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: > > On 09/05/16 13:32, Jani Nikula wrote: > >> On Fri, 06 May 2016, Dave Gordon <david.s.gordon@intel.com> wrote: > >>> On 06/05/16 15:33, Chris Wilson wrote: > >>>> On Fri, May 06, 2016 at 03:16:25PM +0100, Tvrtko Ursulin wrote: > >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>>> > >>>>> To be used for more efficient Gen range checking. > >>>>> > >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>>>> --- > >>>>> drivers/gpu/drm/i915/i915_drv.h | 1 + > >>>>> drivers/gpu/drm/i915/intel_fbc.c | 2 +- > >>>>> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++++++------ > >>>>> 3 files changed, 8 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > >>>>> index 15fcbcece13c..935e381407ba 100644 > >>>>> --- a/drivers/gpu/drm/i915/i915_drv.h > >>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h > >>>>> @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { > >>>>> }) > >>>>> #define INTEL_INFO(p) (&__I915__(p)->info) > >>>>> #define INTEL_GEN(p) (INTEL_INFO(p)->gen) > >>>>> +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) > >>>>> #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) > >>>>> #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >>>>> index d5a7cfec589b..2c3681757aba 100644 > >>>>> --- a/drivers/gpu/drm/i915/intel_fbc.c > >>>>> +++ b/drivers/gpu/drm/i915/intel_fbc.c > >>>>> @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) > >>>>> > >>>>> /* FIXME: We lack the proper locking here, so only run this on the > >>>>> * platforms that need. */ > >>>>> - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) > >>>>> + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) > >>>>> cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); > >>>>> cache->fb.pixel_format = fb->pixel_format; > >>>>> cache->fb.stride = fb->pitches[0]; > >>>>> @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) > >>>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); > >>>>> > >>>>> /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ > >>>>> - if (IS_GEN7(dev)) > >>>>> + if (IS_GEN7(dev_priv)) > >>>>> I915_WRITE(GFX_MODE_GEN7, > >>>>> _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | > >>>>> _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); > >>>>> > >>>>> - if (IS_GEN6(dev)) { > >>>>> + if (IS_GEN6(dev_priv)) { > >>>> > >>>> This chunk shouldn't be in this patch. > >>>> > >>>> Couldn't improve upon INTEL_GEN_RANGE. > >>>> > >>>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >>>> -Chris > >>> > >>> INTEL_GEN_IN_RANGE() ? > >>> > >>> Perhaps emphasises that we're using INclusive ranges? > >> > >> Or just IS_GEN(p, since, until) similar to IS_SKL_REVID() and > >> IS_BXT_REVID(). Might as well make this shorter to write. > >> > >> BR, > >> Jani. > > > > We need a notation that will look good (and be efficient) for "before > > GEN x" and "from GEN y on" (or "up to and including GEN p" and "after > > GEN q") -- these make up the vast majority of tests on the GEN number. > > > > Also, we need real clarity on inclusion/exclusion. since..until notation > > is perhaps ambiguous - in particular, is "until" included? English usage > > is not clear here, it's often but not always exclusive! > > If you want real clarity, you'll ditch this patch and use > > if (INTEL_GEN(p) >= 5 && INTEL_GEN(p) <= 7) > etc... > > and be done with it. There's too much churn going on with all the > related macros anyway, so I'm not at all convinced about the patches > anyway. IS_GEN(p, start, end) using inclusive ranges is fine. This is a small patch that generates a remarkable amount of object code reduction. -Chris
On Mon, 09 May 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > IS_GEN(p, start, end) using inclusive ranges is fine. > > This is a small patch that generates a remarkable amount of object code > reduction. Ok. I guess 0 is okay for start, but how about a "GEN_FOREVER" style macro for replacing ->gen >= 5 with IS_GEN(dev_priv, 5, GEN_FOREVER)? This would be in line with the IS_{SKL,BXT}_REVID checks. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 15fcbcece13c..935e381407ba 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2518,6 +2518,7 @@ struct drm_i915_cmd_table { }) #define INTEL_INFO(p) (&__I915__(p)->info) #define INTEL_GEN(p) (INTEL_INFO(p)->gen) +#define INTEL_GEN_RANGE(p, s, e) (INTEL_INFO(p)->gen_mask & GENMASK(e, s)) #define INTEL_DEVID(p) (INTEL_INFO(p)->device_id) #define INTEL_REVID(p) (__I915__(p)->dev->pdev->revision) diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index d5a7cfec589b..2c3681757aba 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -740,7 +740,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc) /* FIXME: We lack the proper locking here, so only run this on the * platforms that need. */ - if (INTEL_INFO(dev_priv)->gen >= 5 && INTEL_INFO(dev_priv)->gen < 7) + if (INTEL_GEN_RANGE(dev_priv, 5, 6)) cache->fb.ilk_ggtt_offset = i915_gem_obj_ggtt_offset(obj); cache->fb.pixel_format = fb->pixel_format; cache->fb.stride = fb->pitches[0]; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 710cd598c701..c3dbb74d048b 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -508,7 +508,7 @@ static void intel_ring_setup_status_page(struct intel_engine_cs *engine) * arises: do we still need this and if so how should we go about * invalidating the TLB? */ - if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) { + if (INTEL_GEN_RANGE(dev_priv, 6, 7)) { i915_reg_t reg = RING_INSTPM(engine->mmio_base); /* ring should be idle before issuing a sync flush*/ @@ -1222,7 +1222,7 @@ static int init_render_ring(struct intel_engine_cs *engine) return ret; /* WaTimedSingleVertexDispatch:cl,bw,ctg,elk,ilk,snb */ - if (INTEL_INFO(dev)->gen >= 4 && INTEL_INFO(dev)->gen < 7) + if (INTEL_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 @@ -1231,7 +1231,7 @@ static int init_render_ring(struct intel_engine_cs *engine) * * WaDisableAsyncFlipPerfMode:snb,ivb,hsw,vlv */ - if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) + if (INTEL_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 */ @@ -1241,12 +1241,12 @@ static int init_render_ring(struct intel_engine_cs *engine) _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT)); /* WaBCSVCSTlbInvalidationMode:ivb,vlv,hsw */ - if (IS_GEN7(dev)) + if (IS_GEN7(dev_priv)) I915_WRITE(GFX_MODE_GEN7, _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_EXPLICIT) | _MASKED_BIT_ENABLE(GFX_REPLAY_MODE)); - if (IS_GEN6(dev)) { + if (IS_GEN6(dev_priv)) { /* From the Sandybridge PRM, volume 1 part 3, page 24: * "If this bit is set, STCunit will have LRA as replacement * policy. [...] This bit must be reset. LRA replacement @@ -1256,7 +1256,7 @@ static int init_render_ring(struct intel_engine_cs *engine) _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB)); } - if (INTEL_INFO(dev)->gen >= 6 && INTEL_INFO(dev)->gen < 8) + if (INTEL_GEN_RANGE(dev_priv, 6, 7)) I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING)); if (HAS_L3_DPF(dev))