On Wed, Jan 16, 2013 at 10:43:15AM +0100, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 9:17 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
On Tue, Jan 15, 2013 at 06:53:19PM +0100, Daniel Vetter wrote:
On Mon, Jan 14, 2013 at 4:55 PM, Thierry Reding thierry.reding@avionic-design.de wrote:
+static void tegra_drm_preclose(struct drm_device *drm, struct drm_file *file) +{
struct drm_crtc *crtc;list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)tegra_dc_cancel_page_flip(crtc, file);+}
Why that? If userspace dies while a flip is outstanding, it's imo ok to execute it - otherwise there might be an accounting mismatch if the hw still scans out the old fb, but drm believes the new one is used. Or do I miss something?
I looked at the shmobile driver for inspiration and they do this as well. Doing so seemed reasonable since there'd be no file to deliver the event to.
Hm, is the code in drm_events_release not good enough? And if it's buggy, we need to fix it. Also adding Laurent to figure out why he added that code in shmob ...
drm_events_release() should be enough to clean up the events, but I suspect the reason why Laurent put that code in was that the drm_crtc private data still has a reference to the event and needs to clear it. Otherwise the next page flip won't be scheduled because .page_flip() would return -EBUSY.
However, it seems like {tegra_dc,shmob_drm_crtc}_cancel_page_flip() could both be simplified a lot and just set their event to NULL. Then again, maybe keeping a separate reference isn't all that useful. Maybe the better thing to do here is iterate over the list of pending VBLANK events in *_finish_page_flip() and process each of them? That would allow more than one user-space process to queue page flips.
The reason I've skimmed through the patches is to check for implications with my modeset locking rework. Review on that would be highly appreciated ...
I'm not sure how suited I am for review given my limited experience, but I'll see if I can make some time to take a look.
The commit message should nicely explain why I've picked the design and the various implications for drivers. So just checking whether anything collides with your upcoming stuff would be good ...
Okay, I'll take a closer look.
Thierry