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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/07/2018 14:24, Joonas Lahtinen wrote:
> 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.

Requested vs active is for implementing the lazy emit.

AFAIR it does propagate the error out of execbuf (although we never ever 
expect it to happen), and this is just to keep the internal 
house-keeping in sync.

Regards,

Tvrtko

>> @@ -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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx