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

[Chris Wilson]

Quoting Tomasz Lis (2018-06-20 16:03:07)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 33bc914..c69dc26 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -258,6 +258,57 @@ intel_lr_context_descriptor_update(struct i915_gem_context *ctx,
>         ce->lrc_desc = desc;
>  }
>  
> +static int emit_set_data_port_coherency(struct i915_request *req, bool enable)
> +{
> +       u32 *cs;
> +       i915_reg_t reg;
> +
> +       GEM_BUG_ON(req->engine->class != RENDER_CLASS);
> +       GEM_BUG_ON(INTEL_GEN(req->i915) < 9);
> +
> +       cs = intel_ring_begin(req, 4);
> +       if (IS_ERR(cs))
> +               return PTR_ERR(cs);
> +
> +       if (INTEL_GEN(req->i915) >= 10)
> +               reg = CNL_HDC_CHICKEN0;
> +       else
> +               reg = HDC_CHICKEN0;
> +
> +       /* FIXME: this feature may be unuseable on CNL; If this checks to be
> +        *  true, we should enodev for CNL. */
> +       *cs++ = MI_LOAD_REGISTER_IMM(1);
> +       *cs++ = i915_mmio_reg_offset(reg);
> +       /* Enabling coherency means disabling the bit which forces it off */
> +       if (enable)
> +               *cs++ = _MASKED_BIT_DISABLE(HDC_FORCE_NON_COHERENT);
> +       else
> +               *cs++ = _MASKED_BIT_ENABLE(HDC_FORCE_NON_COHERENT);
> +       *cs++ = MI_NOOP;
> +
> +       intel_ring_advance(req, cs);
> +
> +       return 0;
> +}

There's nothing specific to the logical ringbuffer context here afaics.
It could have just been done inside the single
i915_gem_context_set_data_port_coherency(). Also makes it clearer that
i915_gem_context_set_data_port_coherency needs struct_mutex.

cmd = HDC_FORCE_NON_COHERENT << 16;
if (!coherent)
	cmd |= HDC_FORCE_NON_COHERENT;
*cs++ = cmd;

Does that read any clearer?

> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 1593194..214e291 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -104,4 +104,8 @@ struct i915_gem_context;
>  
>  void intel_lr_context_resume(struct drm_i915_private *dev_priv);
>  
> +int
> +intel_lr_context_modify_data_port_coherency(struct i915_gem_context *ctx,
> +                                            bool enable);
> +
>  #endif /* _INTEL_LRC_H_ */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 7f5634c..fab072f 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1453,6 +1453,7 @@ struct drm_i915_gem_context_param {
>  #define I915_CONTEXT_PARAM_NO_ERROR_CAPTURE    0x4
>  #define I915_CONTEXT_PARAM_BANNABLE    0x5
>  #define I915_CONTEXT_PARAM_PRIORITY    0x6
> +#define I915_CONTEXT_PARAM_COHERENCY   0x7

DATAPORT_COHERENCY
There are many different caches.

There should be some commentary around here telling userspace what the
contract is.

>  #define   I915_CONTEXT_MAX_USER_PRIORITY       1023 /* inclusive */
>  #define   I915_CONTEXT_DEFAULT_PRIORITY                0
>  #define   I915_CONTEXT_MIN_USER_PRIORITY       -1023 /* inclusive */

COHERENCY has MAX/MIN_USER_PRIORITY, interesting. I thought it was just
a boolean.
-Chris