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

[Tvrtko Ursulin]

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.

So my patch fixes the issue of struct_mutex not being held in 
intel_alloc_initial_plane_obj while calling 
i915_gem_object_create_stolen_for_preallocated and I'll leave 
struct_mutex/rps.lock locking inversion in RPM to someone else.

Regards,

Tvrtko