[Intel-gfx] [PATCH 21/21] drm/vmwgfx: Nuke preclose hook

[Daniel Vetter]

On Sun, Jan 10, 2016 at 9:52 PM, Thomas Hellstrom <thomas at shipmail.org> wrote:
> On 01/09/2016 11:43 AM, Daniel Vetter wrote:
>> On Fri, Jan 8, 2016 at 9:53 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
>>> On 01/08/2016 09:36 PM, Daniel Vetter wrote:
>>>> Again since the drm core takes care of event unlinking/disarming this
>>>> is now just needless code.
>>>>
>>>> Cc: Thomas Hellström <thellstrom at vmware.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>>> Hmm,
>>>
>>> IIRC this is actually a list of events that core drm is not aware of
>>> yet. They sit on this list waiting for a fence to pass and are then
>>> transferred to core drm....
>> Yes I know. Earlier patches in the series extract new core functions
>> to setup/tear down such events and send them out, which is what's
>> needed to make this trick possible. Exynos similarly uses events
>> similarly, and is also converted. Same for nouveau it seems, but there
>> the code doesn't use the reserve/send split, so I'm unclear
>> how/whether at all it correctly handles this race.
>> -Daniel
>
> Ah. Hmm I should've looked more closely at the rest of the series.
>
> In any case, this particular patch leaves, from what I can tell, the
> eaction fpriv list intact when it is later freed in postclose, which is
> bad. Also each eaction is left with a dangling pointer to a freed
> pending event which is also very bad since that pointer will be
> dereferenced as soon as the fence's seqno has passed. So as far as i can
> tell, this function needs to remain except for the event destruction.

Oops, I missed that part, it needs to go too. All other drivers look
for events to clean up from some other objects (for the crtc page flip
stuff) or are just outright confused, I didn't check vmwgfx code
carefully enough.

> Thinking of it, this must be a problem that is more general problem than
> for vmwgfx only, I mean, unless the driver traverses all core pending
> event list to find relevant pending events to process, something in the
> driver must actually point to the pending event (be it a pointer in the
> fence object or, as in the vmwgfx case, a pointer in the fence action
> object) and that pointer must somehow be invalidated when the pending
> event is freed...

This is exactly what this patch series attempts to do, by unlinking
events from the fpriv if that disappears. Driver code can still call
drm_even_send&friends (which are all rolled out in this series), they
will simply only free up the event and not try to send it out.

> Which also brings up a question, where are the pending events actually
> destroyed? I can see they are unlinked in drm_fops.c.

When the driver eventually calls drm_send_event. That way fpriv
disappearing is completely transparent to the driver.

I'll resend the vmwgfx patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch