[Intel-gfx] [PATCH 7/7] drm/i915: GEM operations need to be done under the big lock

[Tvrtko Ursulin]

On 13/01/16 14:41, Imre Deak wrote:
> On ke, 2016-01-13 at 14:32 +0000, Chris Wilson wrote:
>> On Wed, Jan 13, 2016 at 02:11:42PM +0000, Tvrtko Ursulin wrote:
>>>
>>> On 13/01/16 13:36, Imre Deak wrote:
>>>> On ke, 2016-01-13 at 12:46 +0000, Tvrtko Ursulin wrote:
>>>>> On 11/01/16 16:56, Chris Wilson wrote:
>>>>>> On Mon, Jan 11, 2016 at 05:36:46PM +0200, Ville Syrjälä
>>>>>> wrote:
>>>>>>> On Mon, Jan 11, 2016 at 03:16:16PM +0000, Tvrtko Ursulin
>>>>>>> wrote:
>>>>>>>> Don't know, I leave this one to whoever grabbed the lock
>>>>>>>> around
>>>>>>>> intel_init_gt_powersave in the first place. Maybe there
>>>>>>>> was a
>>>>>>>> special
>>>>>>>> reason.. after git blame od intel_display.c eventually
>>>>>>>> completed, adding
>>>>>>>> Imre and Ville to cc.
>>>>>>>
>>>>>>> Hmm. I don't recall the details anymore, but looking at the
>>>>>>> code
>>>>>>> pushing
>>>>>>> the locking down to valleyview_setup_pctx() looks entirely
>>>>>>> reasonable to
>>>>>>> me.
>>>>>>
>>>>>> iirc, this locking only exists to keep the WARN() at bay. But
>>>>>> it is
>>>>>> pedagogical, I guess.
>>>>>
>>>>> Don't really know this area, but what about the
>>>>> intel_gen6_powersave_work->valleyview_enable_rps-
>>>>>> valleyview_check_pctx
>>>>> which dereferences the dev_priv->vlv_pctx, which is set/cleared
>>>>> in
>>>>> valleyview_setup_pctx/valleyview_cleanup_pctx, which would now
>>>>> be
>>>>> outside both struct_mutex and the rps lock?
>>>>
>>>> dev_priv->vlv_pctx is not protected on the premise that the
>>>> driver
>>>> init/cleanup functions can't race and gen6_powersave_work() is
>>>> scheduled only after intel_init_gt_powersave() and flushed
>>>> before intel_cleanup_gt_powersave().
>>>>
>>>> rps_lock protects the RPS HW accesses themselves and struct_mutex
>>>> was
>>>> taken for the GEM allocation. Taking it at high level around
>>>> intel_init_gt_powersave() was kind of a copy&paste in the commit
>>>> you
>>>> found, there is more on that in 79f5b2c75992.
>>>
>>> Thanks for digging this out.
>>>
>>> It is more involved than what Chris pasted then, and while I hoped
>>> to be able to quickly shove that into a patch, I cannot allow the
>>> time for more the extensive analysis.
>>
>> Imre just confirmed that struct_mutex is only for the GEM
>> allocations in the vlv_setup_ctx, right Imre?
>
> Yes, that's my understanding.

I glanced at 79f5b2c75992 and saw it pulled out a bunch of locking from 
lower down to the top level so it sounded like doing the reverse would 
not be straightforward. Perhaps I overestimated it.

Regards,

Tvrtko