[Intel-gfx] [PATCH v6] drm/i915: Add IOCTL Param to control data port coherency.

[Joonas Lahtinen]

Quoting Tomasz Lis (2018-07-16 16:07:16)
> +static int emit_set_data_port_coherency(struct i915_request *rq, bool enable)
> +{
> +       u32 *cs;
> +       i915_reg_t reg;
> +
> +       GEM_BUG_ON(rq->engine->class != RENDER_CLASS);
> +       GEM_BUG_ON(INTEL_GEN(rq->i915) < 9);
> +
> +       cs = intel_ring_begin(rq, 4);
> +       if (IS_ERR(cs))
> +               return PTR_ERR(cs);
> +
> +       if (INTEL_GEN(rq->i915) >= 11)
> +               reg = ICL_HDC_MODE;
> +       else if (INTEL_GEN(rq->i915) >= 10)
> +               reg = CNL_HDC_CHICKEN0;
> +       else
> +               reg = HDC_CHICKEN0;
> +
> +       *cs++ = MI_LOAD_REGISTER_IMM(1);
> +       *cs++ = i915_mmio_reg_offset(reg);
> +       /* Enabling coherency means disabling the bit which forces it off */

This comment is still spurious, please get rid of the habit of writing
comments about "what" the code is doing, useful comments should be
limited to "why", which is quite self explanatory here, that's the way
the register is.

> +static int
> +intel_lr_context_update_data_port_coherency(struct i915_request *rq)
> +{
> +       struct i915_gem_context *ctx = rq->gem_context;
> +       bool enable = test_bit(CONTEXT_DATA_PORT_COHERENT_REQUESTED, &ctx->flags);
> +       int ret;
> +
> +       lockdep_assert_held(&rq->i915->drm.struct_mutex);
> +
> +       if (test_bit(CONTEXT_DATA_PORT_COHERENT_ACTIVE, &ctx->flags) == enable)
> +               return 0;
> +
> +       ret = emit_set_data_port_coherency(rq, enable);
> +
> +       if (!ret) {
> +               if (enable)
> +                       __set_bit(CONTEXT_DATA_PORT_COHERENT_ACTIVE, &ctx->flags);
> +               else
> +                       __clear_bit(CONTEXT_DATA_PORT_COHERENT_ACTIVE, &ctx->flags);
> +       }

Do we have indication that the hardware feature will be unreliable in
responding to the requests? I don't think you need the differentiation
of requested vs. active. If there is an error, we can just report back to
the user as a failed IOCTL. Now it adds unnecessary complication for no benefit.

> @@ -2164,6 +2221,13 @@ static int gen8_emit_flush_render(struct i915_request *request,
>                 /* WaForGAMHang:kbl */
>                 if (IS_KBL_REVID(request->i915, 0, KBL_REVID_B0))
>                         dc_flush_wa = true;
> +
> +               /* Emit the switch of data port coherency state if needed */

Ditto for spurious comment, just about what the code does.

> +++ b/include/uapi/drm/i915_drm.h
> @@ -1456,6 +1456,13 @@ struct drm_i915_gem_context_param {
>  #define   I915_CONTEXT_MAX_USER_PRIORITY       1023 /* inclusive */
>  #define   I915_CONTEXT_DEFAULT_PRIORITY                0
>  #define   I915_CONTEXT_MIN_USER_PRIORITY       -1023 /* inclusive */
> +/*
> + * When data port level coherency is enabled, the GPU will update memory
> + * buffers shared with CPU, by forcing internal cache units to send memory
> + * writes to higher level caches faster. Enabling data port coherency has
> + * a performance cost.
> + */

I was under impression this is enabled by default and it can be disabled
for a performance optimization?

Regards, Joonas