drm/i915: Update driver to Linux 3.8 (February 2013)
ClosedPublic
Actions

Authored by dumbbell on Mar 4 2016, 5:37 PM.

Details

Reviewers

kib
jhb

Commits

rS296548: drm/i915: Update to match Linux 3.8.13

Summary

After this update, DRM will be consistent and match Linux 3.8.

This i915 update includes initial support for Haswell.

I'm not asking for a review of the entire patch. However, I'm interested in comments about specific areas:

I updated the page fault handler (i915_gem_pager_fault() in i915_gem.c) to be close to Linux handler. I mean, our fault handler calls the same i915 functions than the Linux one. The logic w.r.t. our VM subsystem is unmodified.
Still about the page fault handler, I added a comment in i915_gem_pager_ctor() to explain how we manage references to GEM object, but I'm interested in a confirmation I understand the difference between Linux and FreeBSD correctly.
I would like a review of __wait_seqno() in i915_gem.c because apparently, I can't write it properly.

There are probably more areas I would like comments about, but I don't remember them right now. I will update this review once I remember.

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2776
Build 2797: arc lint + arc unit

dumbbell retitled this revision from to drm/i915: Update driver to Linux 3.8 (February 2013).

dumbbell updated this object.

dumbbell edited the test plan for this revision. (Show Details)

dumbbell added reviewers: jhb, kib.

kib added inline comments.Mar 5 2016, 6:15 PM

sys/dev/drm2/i915/i915_gem.c
1448	This is mostly correct, except that the dtor() is not called on unmapping, generally. The mapping is shared on fork, and dtor is called only on the _last_ mapping destruction.
1483	Using bool for pinned is more logical, isn't it ?
1484	This is very wrong and probably cause random hangs due to userspace asynchronously modifying buffers containing either commands or shaders. FreeBSD currently always pass PROT_READ as prot. I have a patch to provide the information for very long time, but alc did not liked it and thing just stopped. Always assuming write is somewhat not optimal, even more so for hsw+, but at least it is correct.
1509	You may finally remove the cause. It was used for in-field debugging of the fault handler, but I did not used it for several years.
1538	I do not understand what this diff was generated against. If i915_intr_pf is not set, code must do DRM_LOCK(dev), i.e. uninterruptible dev lock acquisition. Otherwise, drm lock is not owned for all later calls to i915_gem functions.

Address kib@'s comments

Also, restore all M_WAITOK in many malloc(9) calls.

Thanks for your comments. All of them should be addressed now.

sys/dev/drm2/i915/i915_gem.c
1538	Phabricator's way to display the diff was incorrect and gave the impression some of the code was missing. In the real file, this part doesn't change: the GEM object is locked in the `else` part. Hopefully the new patch is displayed correctly.