From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH 7/9] drm/omap: fix omap_crtc_flush() to handle the workqueue Date: Tue, 9 Sep 2014 10:07:46 +0300 Message-ID: <540EA742.3080109@ti.com> References: <1409745310-19092-1-git-send-email-tomi.valkeinen@ti.com> <1409745310-19092-7-git-send-email-tomi.valkeinen@ti.com> <20140903142750.GT15520@phenom.ffwll.local> <540DA916.8030601@ti.com> <20140908133139.GZ15520@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0264533156==" Return-path: Received: from bear.ext.ti.com (bear.ext.ti.com [192.94.94.41]) by gabe.freedesktop.org (Postfix) with ESMTP id DEEC66E12B for ; Tue, 9 Sep 2014 00:08:04 -0700 (PDT) In-Reply-To: <20140908133139.GZ15520@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0264533156== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LUD69El5vAtablqTVGKnPEpIB84LP7eVr" --LUD69El5vAtablqTVGKnPEpIB84LP7eVr Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 08/09/14 16:31, Daniel Vetter wrote: > On Mon, Sep 08, 2014 at 04:03:18PM +0300, Tomi Valkeinen wrote: >> On 03/09/14 17:27, Daniel Vetter wrote: >>> On Wed, Sep 03, 2014 at 02:55:08PM +0300, Tomi Valkeinen wrote: >>>> omap_crtc_flush() is used to wait for scheduled work to be done for = the >>>> give crtc. However, it's not quite right at the moment. >>>> >>>> omap_crtc_flush() does wait for work that is ran via vsync irq to be= >>>> done. However, work is also queued to the driver's priv->wq workqueu= e, >>>> which is not handled by omap_crtc_flush(). >>>> >>>> Improve omap_crtc_flush() to flush the workqueue so that work there = will >>>> be ran. >>>> >>>> This fixes a race issue on module unload, where an unpin work may be= on >>>> the work queue, but does not get ran before drm core starts tearing = the >>>> driver down, leading to a WARN. >>>> >>>> Signed-off-by: Tomi Valkeinen >>> >>> I didn't really dig into details, but isn't that the same workqueue a= s >>> used by the async modeset code? So the same deadlocks might happen ..= =2E >> >> Yes, we have just one workqueue in the driver. >> >> Hmm, deadlocks with what lock? The modeconfig or crtc->mutex? I don't >> think they are locked at any place where omap_crtc_flush is called. >=20 > Oh, I presumed you're using _flush in the relevant modeset functions - = we No. That's the locking issue again. We can't flush when holding the crtc mutex, as the works in the workqueue also try to grab it... > do that in i915 to make sure that all the pageflips and other stuff > completed before we do another modeset. But omap only calls this at dri= ver > unload, so no direct problem. At the moment yes, but in this series I add the same omap_crtc_flush() call to two new places: dev_preclose and omap_crtc_commit. Of which the omap_crtc_commit is the problematic one, discussed in the mail thread for patch 4. >>> lockdep won't complain though since you essentially open-code a >>> workqueue_flush, and lockdep also doesn't complain about all possible= >>> deadlocks (due to some design issues with lockdep). >> >> What do you mean "open-code a workqueue_flush"?. I use flush_workqueue= >> there. We have two things to wait for: work on the workqueue and work >> which is triggered by the vsync irq. So we loop and test for both of >> those, until there's no more work. >=20 > Oops, missed that. Ordering looks wrong though since if the irq can lat= ch > the workqueue you need to wait for irqs to happen first before flushing= =2E > And obviously queue the work before signalling the completion of the > interrupt. But since this seems to lack locking anyway and is only used= > for unload it doesn't really matter. Yeah, well, the workqueue can create work for the irq also. I don't know if it does, currently, but I think it's safer to presume that both workqueue and the irq can create work to the other. But that's why I have a loop there. So we flush, then check if there is work for the irq. If yes, sleep a bit and go back to start. So if the irq work created new work for the wq, we flush that. And if that work created new work for the irq, we check that again. Etc. Tomi --LUD69El5vAtablqTVGKnPEpIB84LP7eVr Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUDqdCAAoJEPo9qoy8lh71OSQP/A777SQy8H+OdlNbCv4svTnH lR8RxPqvcAQTRpfzpsESPgAP13Ut473QdZJWOyHrDvQIHDQzLLVglEftRnvUHa9Q tkYyhOD+SYxnBf5q0EVaS8lffHpRQe7Da52o0wKTeOXi/VCYTDJKgaWe55Mk88z0 riFMaxz8taE6AlcoM4zY3OcnX1T2i/CZEEmqoUCwCo50k1ul0g6pU9uZ3hTmNhkQ 2THQw5Hxg3LFz5JaOrVR/ZLuTEbPOHRq2rbwxK/2F01ld0m+MUhZqHrtnpcv69zG Khvww1ri1xEX/QsSPDdEY6Id+lLWnu/vacTeBfVRPMTcfdLcEnGca3NZCuZtmH55 4I7Qy3soWED0zxduMv0vyj60jZZSQU2HxYDQ/okl87mlPJg+MeowaaG902sNkJ0F YnpE9vVwS6t4Bol4YzJ15b+VWyz0TynwjttFGfxw71BtsRLmipAQkFmj+Evea6GK u23dc5huiZloyL3t1rbw+IJWXE3WwsFNsVUtPhHVrgCHKVkf2nuaWsBATwqZEo/h DGF/7VANE7uH+aT/0vE2eMjT71rh7k5uPmh8WqYPI/fSwtz0jzO9US44b8MhjUi9 fTKaE2L9P/qafpIHl+vZ7h/O1rGBa8PC1RFRygZGPjHC9zAMEzwOXl3e9R/w094b PWbwhiP5eXTszczLZACa =RQ6W -----END PGP SIGNATURE----- --LUD69El5vAtablqTVGKnPEpIB84LP7eVr-- --===============0264533156== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0264533156==--