Re: [PATCH v4 2/2] drm/i915: kill resource streamer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2018-08-06 16:31:48)
> 
> On 04/08/2018 00:24, 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 RS years ago, but nothing ever landed or shipped
> > because there was no performance improvement.
> > 
> > This removes it from kernel keeping the uapi defines around for
> > compatibility.
> > 
> > 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)
> > 
> > v3: - disable CTX_CTRL_RS_CTX_ENABLE istead of removing it
> >      - reword commit message after Daniele confirmed no performance
> >        regression on his machine
> >      - reword commit message to make clear RS is being removed due to
> >        never been used
> > v4: - move I915_EXEC_RESOURCE_STREAMER to __I915_EXEC_ILLEGAL_FLAGS so
> >        the check on ioctl() is made much earlier by
> >        i915_gem_check_execbuffer() (suggested by Tvrtko)
> > 
> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@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 | 17 ++---------------
> >   drivers/gpu/drm/i915/i915_pci.c            |  4 ----
> >   drivers/gpu/drm/i915/intel_device_info.h   |  1 -
> >   drivers/gpu/drm/i915/intel_lrc.c           | 10 ++++------
> >   drivers/gpu/drm/i915/intel_ringbuffer.c    |  4 +---
> >   drivers/gpu/drm/i915/intel_ringbuffer.h    |  1 -
> >   8 files changed, 8 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 64e0ea4bef67..3857e7963fc5 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 4aca5344863d..657f46e0cae9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2610,8 +2610,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..06bb434644e5 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -64,7 +64,8 @@ enum {
> >   #define BATCH_OFFSET_BIAS (256*1024)
> >   
> >   #define __I915_EXEC_ILLEGAL_FLAGS \
> > -     (__I915_EXEC_UNKNOWN_FLAGS | I915_EXEC_CONSTANTS_MASK)
> > +     (__I915_EXEC_UNKNOWN_FLAGS | I915_EXEC_CONSTANTS_MASK | \
> > +      I915_EXEC_RESOURCE_STREAMER)
> >   
> >   /* Catch emission of unexpected errors for CI! */
> >   #if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> > @@ -2221,20 +2222,6 @@ 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_FENCE_IN) {
> >               in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2));
> >               if (!in_fence)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 8a9a9009db62..e931b48369dd 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -368,7 +368,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
> > @@ -441,7 +440,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,
> > @@ -515,7 +513,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, \
> > @@ -604,7 +601,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 07e8364d1a8c..6eecd64734d5 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(has_coherent_ggtt); \
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index b0be180c6294..e5385dbfcdda 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -2065,8 +2065,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);
> >   
> > @@ -2584,10 +2583,9 @@ 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_CTRL_RS_CTX_ENABLE) |
> > +             _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 80a8b6e57374..8003cef767ba 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1980,9 +1980,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 57f3787ed6ec..8837079cb8b3 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -474,7 +474,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;
> >   
> > 
> 
> I don't know if it is true no one uses the feature - Google search 
> suggest no open source users at least. But removal looks correct so:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

And pushed. The only residual bit is the selection of extended state for
Haswell. I think that should be set unconditionally and claiming the
extended state is only needed for RS is a red herring...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux