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

[Lis, Tomasz]

On 2018-06-21 09:05, Chris Wilson wrote:
> 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?
Sorry, I don't think I follow.
Should I move the code out of logical ringbuffer context (intel_lrc.c)?
Should I merge the emit_set_data_port_coherency() with 
intel_lr_context_modify_data_port_coherency()?
Should I lock a mutex while adding the request?
-Tomasz
>
>> 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.
Will do.
>
>>   #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
I did not noticed the structure of defines here; will move the new define.
-Tomasz