On Thu, Jul 12, 2018 at 04:48:42PM -0700, Rodrigo Vivi wrote:
> On Thu, Jul 12, 2018 at 11:28:25PM +0000, De Marchi, Lucas wrote:
> > On Thu, 2018-07-12 at 16:06 -0700, Rodrigo Vivi wrote:
> > > On Thu, Jul 12, 2018 at 02:02:51PM -0700, Lucas De Marchi wrote:
> > > > After disabling resource streamer on ICL (due to it actually not
> > > > existing there), I got feedback that there have been some experimental
> > > > patches for mesa to use it, but nothing ever landed nor shipped.
> > > >
> > > > This is a tentative to remove it from kernel keeping the uapi defines
> > > > around for compatibility. We may need to keep CTX_CTRL_RS_CTX_ENABLE on
> > > > some platforms - Daniele mentioned there's possible performance regression
> > > > in gem_latency if this bit is not set. On this patch, I'm just removing
> > > > it in order to get proper numbers.
> > >
> > > Since I see below 3 occurrences of
> > > - .has_resource_streamer = 1
> > >
> > > I feel this commit message lacks of the reason why we are removing
> > > this features for the platforms that already had it.
> >
> > /me confused. Isn't the first paragraph in the commit message exactly that?
>
> /me confused now... For me the first paragraph tells that after removing
> from ICL mesa might want to use it on ICL.
>
> second paragraph tells that you are removing the support but keeping the uapi.
>
> what I couldn't find is the explanation why exactly you are removing this
> feature. Is this useless? No body using it? It is buggy? It is deprecated? why?
I guess a small change should fix the meaning of the first para:
"... there have been some experimental patches for mesa to use it years
ago, but nothing ever landed or shipped because there was no performance
improvement."
These experimental patches long stopped existing :-)
Also note that the uapi doesn't stay around, only the #defines (we can't
ever reuse them for anything else than RS, so need to keep them reserved).
Cheers, Daniel
>
> >
> > Lucas De Marchi
> >
> > >
> > > >
> > > > v2: - re-add the inadvertent removal of CTX_CTRL_INHIBIT_SYN_CTX_SWITCH
> > > > - don't bother trying to document removed params on uapi header:
> > > > applications should know that from the query.
> > > > (from Chris)
> > > >
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c | 2 +-
> > > > drivers/gpu/drm/i915/i915_drv.h | 2 --
> > > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 15 ++-------------
> > > > drivers/gpu/drm/i915/i915_pci.c | 4 ----
> > > > drivers/gpu/drm/i915/intel_device_info.h | 1 -
> > > > drivers/gpu/drm/i915/intel_lrc.c | 7 ++-----
> > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +---
> > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> > > > 8 files changed, 6 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 3eba3d1ab5b8..10423e2c1176 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -373,7 +373,7 @@ static int i915_getparam_ioctl(struct drm_device *dev,
> > > > void *data,
> > > > value = 2;
> > > > break;
> > > > case I915_PARAM_HAS_RESOURCE_STREAMER:
> > > > - value = HAS_RESOURCE_STREAMER(dev_priv);
> > > > + value = 0;
> > > > break;
> > > > case I915_PARAM_HAS_POOLED_EU:
> > > > value = HAS_POOLED_EU(dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 01dd29837233..dddb886fa06c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -2609,8 +2609,6 @@ intel_info(const struct drm_i915_private *dev_priv)
> > > > #define USES_GUC_SUBMISSION(dev_priv) intel_uc_is_using_guc_submis
> > > > sion()
> > > > #define USES_HUC(dev_priv) intel_uc_is_using_huc()
> > > >
> > > > -#define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)-
> > > > >info.has_resource_streamer)
> > > > -
> > > > #define HAS_POOLED_EU(dev_priv) ((dev_priv)->info.has_pooled_eu)
> > > >
> > > > #define INTEL_PCH_DEVICE_ID_MASK 0xff80
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > index 1932bc227942..ca21a08b2be9 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > @@ -2221,19 +2221,8 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > > if (!eb.engine)
> > > > return -EINVAL;
> > > >
> > > > - if (args->flags & I915_EXEC_RESOURCE_STREAMER) {
> > > > - if (!HAS_RESOURCE_STREAMER(eb.i915)) {
> > > > - DRM_DEBUG("RS is only allowed for Haswell and
> > > > Gen8 - Gen10\n");
> > > > - return -EINVAL;
> > > > - }
> > > > - if (eb.engine->id != RCS) {
> > > > - DRM_DEBUG("RS is not available on %s\n",
> > > > - eb.engine->name);
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - eb.batch_flags |= I915_DISPATCH_RS;
> > > > - }
> > > > + if (args->flags & I915_EXEC_RESOURCE_STREAMER)
> > > > + return -EINVAL;
> > > >
> > > > if (args->flags & I915_EXEC_FENCE_IN) {
> > > > in_fence = sync_file_get_fence(lower_32_bits(args-
> > > > >rsvd2));
> > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > > > b/drivers/gpu/drm/i915/i915_pci.c
> > > > index c03ba0fe0845..7af19097dce8 100644
> > > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > > @@ -360,7 +360,6 @@ static const struct intel_device_info
> > > > intel_valleyview_info = {
> > > > .has_ddi = 1, \
> > > > .has_fpga_dbg = 1, \
> > > > .has_psr = 1, \
> > > > - .has_resource_streamer = 1, \
> > > > .has_dp_mst = 1, \
> > > > .has_rc6p = 0 /* RC6p removed-by HSW */, \
> > > > .has_runtime_pm = 1
> > > > @@ -433,7 +432,6 @@ static const struct intel_device_info
> > > > intel_cherryview_info = {
> > > > .ring_mask = RENDER_RING | BSD_RING | BLT_RING | VEBOX_RING,
> > > > .has_64bit_reloc = 1,
> > > > .has_runtime_pm = 1,
> > > > - .has_resource_streamer = 1,
> > > > .has_rc6 = 1,
> > > > .has_logical_ring_contexts = 1,
> > > > .has_gmch_display = 1,
> > > > @@ -506,7 +504,6 @@ static const struct intel_device_info
> > > > intel_skylake_gt4_info = {
> > > > .has_runtime_pm = 1, \
> > > > .has_pooled_eu = 0, \
> > > > .has_csr = 1, \
> > > > - .has_resource_streamer = 1, \
> > > > .has_rc6 = 1, \
> > > > .has_dp_mst = 1, \
> > > > .has_logical_ring_contexts = 1, \
> > > > @@ -593,7 +590,6 @@ static const struct intel_device_info
> > > > intel_cannonlake_info = {
> > > > GEN(11), \
> > > > .ddb_size = 2048, \
> > > > .has_csr = 0, \
> > > > - .has_resource_streamer = 0, \
> > > > .has_logical_ring_elsq = 1
> > > >
> > > > static const struct intel_device_info intel_icelake_11_info = {
> > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.h
> > > > b/drivers/gpu/drm/i915/intel_device_info.h
> > > > index 633f9fbf72ea..6814cc7dfcd3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > > > @@ -103,7 +103,6 @@ enum intel_platform {
> > > > func(has_psr); \
> > > > func(has_rc6); \
> > > > func(has_rc6p); \
> > > > - func(has_resource_streamer); \
> > > > func(has_runtime_pm); \
> > > > func(has_snoop); \
> > > > func(unfenced_needs_alignment); \
> > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > index 35d37af0cb9a..ad10fb7b9668 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > @@ -2127,8 +2127,7 @@ static int gen8_emit_bb_start(struct i915_request
> > > > *rq,
> > > >
> > > > /* FIXME(BDW): Address space and security selectors. */
> > > > *cs++ = MI_BATCH_BUFFER_START_GEN8 |
> > > > - (flags & I915_DISPATCH_SECURE ? 0 : BIT(8)) |
> > > > - (flags & I915_DISPATCH_RS ? MI_BATCH_RESOURCE_STREAMER :
> > > > 0);
> > > > + (flags & I915_DISPATCH_SECURE ? 0 : BIT(8));
> > > > *cs++ = lower_32_bits(offset);
> > > > *cs++ = upper_32_bits(offset);
> > > >
> > > > @@ -2647,9 +2646,7 @@ static void execlists_init_reg_state(u32 *regs,
> > > > CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(engine),
> > > > _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> > > > CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT) |
> > > > - _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
> > > > - (HAS_RESOURCE_STREAMER(dev_priv) ?
> > > > - CTX_CTRL_RS_CTX_ENABLE : 0)));
> > > > + _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH));
> > > > CTX_REG(regs, CTX_RING_HEAD, RING_HEAD(base), 0);
> > > > CTX_REG(regs, CTX_RING_TAIL, RING_TAIL(base), 0);
> > > > CTX_REG(regs, CTX_RING_BUFFER_START, RING_START(base), 0);
> > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > index f4bd185c9369..e5fcbbc2abbc 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > > @@ -1977,9 +1977,7 @@ hsw_emit_bb_start(struct i915_request *rq,
> > > > return PTR_ERR(cs);
> > > >
> > > > *cs++ = MI_BATCH_BUFFER_START | (dispatch_flags &
> > > > I915_DISPATCH_SECURE ?
> > > > - 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW) |
> > > > - (dispatch_flags & I915_DISPATCH_RS ?
> > > > - MI_BATCH_RESOURCE_STREAMER : 0);
> > > > + 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW);
> > > > /* bit0-7 is the length on GEN6+ */
> > > > *cs++ = offset;
> > > > intel_ring_advance(rq, cs);
> > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > index d1eee08e5f6b..a2c1d7044f2a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > > > @@ -467,7 +467,6 @@ struct intel_engine_cs {
> > > > unsigned int dispatch_flags);
> > > > #define I915_DISPATCH_SECURE BIT(0)
> > > > #define I915_DISPATCH_PINNED BIT(1)
> > > > -#define I915_DISPATCH_RS BIT(2)
> > > > void (*emit_breadcrumb)(struct i915_request *rq,
> > > > u32 *cs);
> > > > int emit_breadcrumb_sz;
> > > >
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
