On Fri, Jul 13, 2018 at 10:46:26AM +0200, Daniel Vetter wrote:
> 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.
nah, the *hw* block doesn't exist on ICL, a patch to use it there would
be garbage, not experimental :)
> >
> > second paragraph tells that you are removing the support but keeping the uapi.
by "use it" I was referring to the feature itself.
> >
> > 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."
yep, it's better. And s/it/RS/ would help clear out the confusion on my
badly worded paragraph.
>
> 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).
I think I got it right on this one saying "keeping the uapi *defines*
around for compatibility.
Lucas De Marchi
>
> 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
