[Intel-gfx] [PATCH] drm/i915: Check engine->default_state mapping on module load

[Tvrtko Ursulin]

On 14/09/2018 11:52, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-14 11:03:18)
>>
>> On 14/09/2018 10:51, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-09-14 10:43:12)
>>>>
>>>> On 14/09/2018 10:21, Chris Wilson wrote:
>>>>> Check we can indeed acquire a WB mapping of the context image on module
>>>>> load. Later this will give us the opportunity to validate that we can
>>>>> switch from WC to WB as required.
>>>>>
>>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/i915_gem.c | 11 +++++++++++
>>>>>     1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>>> index be9d012d851b..37353afec66e 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>> @@ -5424,6 +5424,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>>>     
>>>>>         for_each_engine(engine, i915, id) {
>>>>>                 struct i915_vma *state;
>>>>> +             void *vaddr;
>>>>>     
>>>>>                 state = to_intel_context(ctx, engine)->state;
>>>>>                 if (!state)
>>>>> @@ -5446,6 +5447,16 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>>>                         goto err_active;
>>>>>     
>>>>>                 engine->default_state = i915_gem_object_get(state->obj);
>>>>> +
>>>>> +             /* Check we can acquire the image of the context state */
>>>>> +             vaddr = i915_gem_object_pin_map(engine->default_state,
>>>>> +                                             I915_MAP_WB);
>>>>> +             if (IS_ERR(vaddr)) {
>>>>> +                     err = PTR_ERR(vaddr);
>>>>> +                     goto err_active;
>>>>> +             }
>>>>> +
>>>>> +             i915_gem_object_unpin_map(engine->default_state);
>>>>
>>>> Ah this perhaps strengthens the argument to have
>>>> __intel_engines_pin_default_state? (And the unpin pair.)
>>>
>>> I hope it doesn't come to making this into an interface :)
>>> Definitely not an idea I endorse, random manipulation of the
>>> default_state.
>>
>> Double underscore alleviates those concerns, no? :)
> 
> Bah, did you see the EXPORT_SYMBOL_GPL(__i915_gem_object_set_pages),
> nothing is safe!
>   
>> Would makes it only one place the following patch needed to switch to
>> MAP_FORCE_WB.
> 
> But that was the point of having a separate step here. This is then the
> only mapping of default_state that needs to do the force. I liked that.
> The other one we need is the initialisation of the lrc state.

Hmm true.. makes some sense as much as I like avoiding code duplication. 
Okay..

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko