| Message ID | 20180710000658.4158-3-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Lucas De Marchi (2018-07-10 01:06:58) > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 1932bc227942..a5eec97a40fe 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) > + DRM_DEBUG("RS is removed from all Gens: ignoring.\n"); return -EINVAL; If userspace set up its batch on the assumption that RS is enabled, it will fail. > @@ -2657,10 +2656,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 | Bring back INHIBIT_SYNC_CTX_SWITCH. > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 7f5634ce8e88..179e0477dbc6 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > #define I915_PARAM_SUBSLICE_TOTAL 33 > #define I915_PARAM_EU_TOTAL 34 > #define I915_PARAM_HAS_GPU_RESET 35 > +/* Deprecated: do not use */ Don't bother. As written it implies all subsequent param are deprecated. Userspace knows simply by the query return. > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > #define I915_PARAM_HAS_POOLED_EU 38
On Tue, Jul 10, 2018 at 2:51 AM Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Lucas De Marchi (2018-07-10 01:06:58) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 1932bc227942..a5eec97a40fe 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) > > + DRM_DEBUG("RS is removed from all Gens: ignoring.\n"); > > return -EINVAL; > > If userspace set up its batch on the assumption that RS is enabled, it > will fail. ok, > > > @@ -2657,10 +2656,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 | > > Bring back INHIBIT_SYNC_CTX_SWITCH. ugh, thanks for catching that. > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 7f5634ce8e88..179e0477dbc6 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > > #define I915_PARAM_SUBSLICE_TOTAL 33 > > #define I915_PARAM_EU_TOTAL 34 > > #define I915_PARAM_HAS_GPU_RESET 35 > > +/* Deprecated: do not use */ > > Don't bother. As written it implies all subsequent param are deprecated. > Userspace knows simply by the query return. ok. I'll respin a new version. Lucas De Marchi > > > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > > #define I915_PARAM_HAS_POOLED_EU 38 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 09, 2018 at 05:06:58PM -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. > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > 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 | 8 ++------ > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 - > include/uapi/drm/i915_drm.h | 1 + > tools/include/uapi/drm/i915_drm.h | 1 + > 10 files changed, 8 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 0db3c83cce29..6715e37ee66b 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -370,7 +370,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 bec0a2796c37..bd69e4b120a8 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_submission() > #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..a5eec97a40fe 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) > + DRM_DEBUG("RS is removed from all Gens: ignoring.\n"); Still missing the return -EINVAL here. Needs an igt update I think. > > 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 ab89dabc2965..843a940bcc58 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -2138,8 +2138,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); > > @@ -2657,10 +2656,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))); I'd hard-code this to disable the RS instead of removing it outright. With these 2 nits addressed and the commit message augmented as I propose in my other reply: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); > 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 700f94c371b3..a76b974c398d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1979,9 +1979,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 ce6cc2a6cf7a..efec5b14ddb0 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -472,7 +472,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; > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 7f5634ce8e88..179e0477dbc6 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > #define I915_PARAM_SUBSLICE_TOTAL 33 > #define I915_PARAM_EU_TOTAL 34 > #define I915_PARAM_HAS_GPU_RESET 35 > +/* Deprecated: do not use */ > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > #define I915_PARAM_HAS_POOLED_EU 38 > diff --git a/tools/include/uapi/drm/i915_drm.h b/tools/include/uapi/drm/i915_drm.h > index 7f5634ce8e88..179e0477dbc6 100644 > --- a/tools/include/uapi/drm/i915_drm.h > +++ b/tools/include/uapi/drm/i915_drm.h > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > #define I915_PARAM_SUBSLICE_TOTAL 33 > #define I915_PARAM_EU_TOTAL 34 > #define I915_PARAM_HAS_GPU_RESET 35 > +/* Deprecated: do not use */ > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > #define I915_PARAM_HAS_POOLED_EU 38 > -- > 2.17.1 >
On Fri, Jul 13, 2018 at 10:51:16AM +0200, Daniel Vetter wrote: > On Mon, Jul 09, 2018 at 05:06:58PM -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. > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > > --- > > 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 | 8 ++------ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--- > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 - > > include/uapi/drm/i915_drm.h | 1 + > > tools/include/uapi/drm/i915_drm.h | 1 + > > 10 files changed, 8 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 0db3c83cce29..6715e37ee66b 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -370,7 +370,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 bec0a2796c37..bd69e4b120a8 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_submission() > > #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..a5eec97a40fe 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) > > + DRM_DEBUG("RS is removed from all Gens: ignoring.\n"); > > Still missing the return -EINVAL here. Needs an igt update I think. that's because you are replying on v1 rather than v2. See the updated patch down the thread. > > > > > 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 ab89dabc2965..843a940bcc58 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -2138,8 +2138,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); > > > > @@ -2657,10 +2656,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))); > > I'd hard-code this to disable the RS instead of removing it outright. _MASKED_BIT_DISABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT | CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT | CTX_CTRL_RS_CTX_ENABLE) | _MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH); right? > > With these 2 nits addressed and the commit message augmented as I propose > in my other reply: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> thanks Lucas De Marchi > > > > + CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); > > 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 700f94c371b3..a76b974c398d 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -1979,9 +1979,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 ce6cc2a6cf7a..efec5b14ddb0 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -472,7 +472,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; > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > index 7f5634ce8e88..179e0477dbc6 100644 > > --- a/include/uapi/drm/i915_drm.h > > +++ b/include/uapi/drm/i915_drm.h > > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > > #define I915_PARAM_SUBSLICE_TOTAL 33 > > #define I915_PARAM_EU_TOTAL 34 > > #define I915_PARAM_HAS_GPU_RESET 35 > > +/* Deprecated: do not use */ > > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > > #define I915_PARAM_HAS_POOLED_EU 38 > > diff --git a/tools/include/uapi/drm/i915_drm.h b/tools/include/uapi/drm/i915_drm.h > > index 7f5634ce8e88..179e0477dbc6 100644 > > --- a/tools/include/uapi/drm/i915_drm.h > > +++ b/tools/include/uapi/drm/i915_drm.h > > @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { > > #define I915_PARAM_SUBSLICE_TOTAL 33 > > #define I915_PARAM_EU_TOTAL 34 > > #define I915_PARAM_HAS_GPU_RESET 35 > > +/* Deprecated: do not use */ > > #define I915_PARAM_HAS_RESOURCE_STREAMER 36 > > #define I915_PARAM_HAS_EXEC_SOFTPIN 37 > > #define I915_PARAM_HAS_POOLED_EU 38 > > -- > > 2.17.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 0db3c83cce29..6715e37ee66b 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -370,7 +370,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 bec0a2796c37..bd69e4b120a8 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_submission() #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..a5eec97a40fe 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) + DRM_DEBUG("RS is removed from all Gens: ignoring.\n"); 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 ab89dabc2965..843a940bcc58 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -2138,8 +2138,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); @@ -2657,10 +2656,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))); + CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT)); 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 700f94c371b3..a76b974c398d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1979,9 +1979,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 ce6cc2a6cf7a..efec5b14ddb0 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -472,7 +472,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; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 7f5634ce8e88..179e0477dbc6 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 #define I915_PARAM_HAS_GPU_RESET 35 +/* Deprecated: do not use */ #define I915_PARAM_HAS_RESOURCE_STREAMER 36 #define I915_PARAM_HAS_EXEC_SOFTPIN 37 #define I915_PARAM_HAS_POOLED_EU 38 diff --git a/tools/include/uapi/drm/i915_drm.h b/tools/include/uapi/drm/i915_drm.h index 7f5634ce8e88..179e0477dbc6 100644 --- a/tools/include/uapi/drm/i915_drm.h +++ b/tools/include/uapi/drm/i915_drm.h @@ -449,6 +449,7 @@ typedef struct drm_i915_irq_wait { #define I915_PARAM_SUBSLICE_TOTAL 33 #define I915_PARAM_EU_TOTAL 34 #define I915_PARAM_HAS_GPU_RESET 35 +/* Deprecated: do not use */ #define I915_PARAM_HAS_RESOURCE_STREAMER 36 #define I915_PARAM_HAS_EXEC_SOFTPIN 37 #define I915_PARAM_HAS_POOLED_EU 38
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. Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- 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 | 8 ++------ drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +--- drivers/gpu/drm/i915/intel_ringbuffer.h | 1 - include/uapi/drm/i915_drm.h | 1 + tools/include/uapi/drm/i915_drm.h | 1 + 10 files changed, 8 insertions(+), 31 deletions(-)