On Thu, 27 Oct 2011, Ilija Hadzic wrote:
While locking at the code I also noticed that wait_vblank calls drm_vblank_get, but not always drm_vblank_put. The code is correct, the missing vblank_put is hidden in drm_queue_vblank_event. Can you also write the patch to move that around to not trip up reviewers the next time around?
Actually, there is something fishy here. Look at the 'else' branch under the 'if ((seq - vblwait->request.sequence) <= (1 << 23))' at the end of the drm_queue_vblank_event. It doesn't have the 'drm_vblank_put'. It looks wrong to me, but I need to first convince myself that there is not some other obscure place where drm_vblank_put is accounted for. If that branch is really missing a drm_vblank_put, then it will be easy pull the drm_vblank_put out. Otherwise, it will be another knot to untangle.
OK, I checked this. Although the code is a little hard to read, it is done the way it is with the reason. Whenever the drm_queue_vblank_event and that 'else' branch is caught, the drm_vblank_put should *not* be called. The reason is that the relevant vblank has not come in yet, so the reference must remain up so that the vblank interrupts are not disabled until the event occurs.
The seemingly missing drm_vblank_put will be accounted for in drm_handle_vblank_events.
So I should not move anything around. If anything, this should be annotated with comments to explain what's going on.
-- Ilija