Page MenuHomeFreeBSD

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

Authored by dumbbell on Mar 4 2016, 5:37 PM.
Tags
None
Referenced Files
F81976925: D5554.id14149.diff
Wed, Apr 24, 12:43 AM
F81976890: D5554.id14174.diff
Wed, Apr 24, 12:42 AM
Unknown Object (File)
Mar 15 2024, 6:05 PM
Unknown Object (File)
Mar 15 2024, 6:05 PM
Unknown Object (File)
Mar 15 2024, 6:05 PM
Unknown Object (File)
Mar 15 2024, 6:05 PM
Unknown Object (File)
Mar 11 2024, 9:51 PM
Unknown Object (File)
Jan 2 2024, 11:21 PM

Details

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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.
sys/dev/drm2/i915/i915_gem.c
1448 ↗(On Diff #14082)

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 ↗(On Diff #14082)

Using bool for pinned is more logical, isn't it ?

1484 ↗(On Diff #14082)

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 ↗(On Diff #14082)

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 ↗(On Diff #14082)

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

  • Improve comment in i915_gem_pager_ctor()
  • Use "bool" to "pinned" variable
  • Assume write access in page fault handler
  • Remove the "cause" variable in page fault handler

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

Hi @kib!

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

sys/dev/drm2/i915/i915_gem.c
1544 ↗(On Diff #14149)

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.

This revision was automatically updated to reflect the committed changes.