- 20 Sep, 2012 6 commits
-
-
Chris Wilson authored
Rather than have multiple data structures for describing our page layout in conjunction with the array of pages, we can migrate all users over to a scatterlist. One major advantage, other than unifying the page tracking structures, this offers is that we replace the vmalloc'ed array (which can be up to a megabyte in size) with a chain of individual pages which helps reduce memory pressure. The disadvantage is that we then do not have a simple array to iterate, or to access randomly. The common case for this is in the relocation processing, which will typically fit within a single scatterlist page and so be almost the same cost as the simple array. For iterating over the array, the extra function call could be optimised away, but in reality is an insignificant cost of either binding the pages, or performing the pwrite/pread. v2: Fix drm_clflush_sg() to not invoke wbinvd as well! And fix the trivial compile error from rebasing. Signed-off-by:
Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch>
-
Chris Wilson authored
By using the recently introduced pinning of pages, we can safely drop the mutex in the knowledge that the pages are not going to disappear beneath us, and so we can simplify the code for iterating over the pages. Signed-off-by:
Ben Widawsky <ben@bwidawsk.net> Signed-off-by:
-
Chris Wilson authored
By using the recently introduced pinning of pages, we can safely drop the mutex in the knowledge that the pages are not going to disappear jeneath us, and so we can simplify the code for iterating over the pages. Note: The old code had such complicated page refcounting since it used obj->pages as a micro-optimization if it's there, but that could (before this patch) disappear when we drop the dev->struct_mutex. Hence some manual page refcounting was required for the slow path, complicated by the fact that pages returned by shmem_read_mapping_page already have a pageref, which needs to be dropped again. Signed-off-by:
-
Chris Wilson authored
We need to refcount our pages in order to prevent reaping them at inopportune times, such as when they currently vmapped or exported to another driver. However, we also wish to keep the lazy deallocation of our pages so we need to take a pin/unpinned approach rather than a simple refcount. Signed-off-by:
-
Chris Wilson authored
In order to specialise functions depending upon the type of object, we can attach vfuncs to each object via a new ->ops pointer. For instance, this will be used in future patches to only bind pages from a dma-buf for the duration that the object is used by the GPU - and so prevent them from pinning those pages for the entire of the object. v2: Bonus comments. Signed-off-by:
-
Ben Widawsky authored
There are internal patches for a feature which require a parameter to query whether support exists . These patches cannot be made external yet. In order to keep existing tests and userspace happy and free from conflicts, reserve a number for it. Signed-off-by:
-
- 19 Sep, 2012 3 commits
-
-
git://people.freedesktop.org/~danvet/drm-intelDave Airlie authored
Daniel writes: "The big ticket item here is the new i915 modeset infrastructure. Shockingly it didn't not blow up all over the place (i.e. I've managed to fix the ugly issues before merging). 1-2 smaller corner cases broke, but we have patches. Also, there's tons of patches on top of this that clean out cruft and fix a few bugs that couldn't be fixed with the crtc helper based stuff. So more stuff to come ;-) Also a few other things: - Tiny fix in the fb helper to go through the official dpms interface instead of calling the crtc helper code. - forcewake code frobbery from Ben, code should be more in-line with what Windows does now. - fixes for the render ring flush on hsw (Paulo) - gpu frequency tracepoint - vlv forcewake changes to better align it with our understanding of the forcewake magic. - a few smaller cleanups" + 2 fixes. * 'for-airlied' of git://people.freedesktop.org/~danvet/drm-intel: (78 commits) drm/i915: fix OOPS in lid_notify drm/i915: correctly update crtc->x/y in set_base drm/fb helper: don't call drm_helper_connector_dpms directly drm/i915: improve modeset state checking after dpms calls drm/i915: add tons of modeset state checks drm/i915: no longer call drm_helper_resume_force_mode drm/i915: disable all crtcs at suspend time drm/i915: push commit_output_state past the crtc/encoder preparing drm/i915: switch the load detect code to the staged modeset config drm/i915: WARN if the pipe won't turn off drm/i915: s/intel_encoder_disable/intel_encoder_noop drm/i915: push commit_output_state past crtc disabling drm/i915: implement new set_mode code flow drm/i915: compute masks of crtcs affected in set_mode drm/i915: use staged outuput config in lvds->mode_fixup drm/i915: use staged outuput config in tv->mode_fixup drm/i915: extract adjusted mode computation drm/i915: move output commit and crtc disabling into set_mode drm/i915: remove crtc disabling special case drm/i915: push crtc->fb update into pipe_set_base ...
-
Dave Airlie authored
We hit this a lot with i915 and although we'd like to engineer things to hit it a lot less, this commit at least makes it consume a few less cycles. from something containing movzwl 0x0(%rip),%r10d to add %r8,%rdx I only noticed it while using perf to profile something else. Reviewed-by:
Dave Airlie <airlied@redhat.com>
-
git://linuxtv.org/pinchartl/fbdevDave Airlie authored
Laurent writes: The SH Mobile DRM driver is now (in my opinion) ready for mainline. It requires GEM and KMS/FB helpers that have been reviewed on the list and tested. Sascha is waiting for them to reach your tree to send a pull request for another new driver. * 'drm-lcdc' of git://linuxtv.org/pinchartl/fbdev: drm: Renesas SH Mobile DRM driver drm: Add NV24 and NV42 pixel formats DRM: Add DRM KMS/FB CMA helper DRM: Add DRM GEM CMA helper drm/edid: limit printk when facing bad edid
-
- 18 Sep, 2012 4 commits
-
-
Laurent Pinchart authored
The SH Mobile LCD controller (LCDC) DRM driver supports the main graphics plane in RGB and YUV formats, as well as the overlay planes (in alpha-blending mode only). Only flat panel outputs using the parallel interface are supported. Support for SYS panels, HDMI and DSI is currently not implemented. Signed-off-by:
Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by:
Sascha Hauer <s.hauer@pengutronix.de>
-
Laurent Pinchart authored
Signed-off-by: -
Lars-Peter Clausen authored
This patchset introduces a set of helper function for implementing the KMS framebuffer layer for drivers which use the DRM GEM CMA helper function. Signed-off-by:
Lars-Peter Clausen <lars@metafoo.de> Tested-by:
-
Sascha Hauer authored
Many embedded drm devices do not have a IOMMU and no dedicated memory for graphics. These devices use CMA (Contiguous Memory Allocator) backed graphics memory. This patch provides helper functions to be able to share the code. The code technically does not depend on CMA as the backend allocator, the name has been chosen because CMA makes for a nice, short but still descriptive function prefix. Signed-off-by:
-
- 17 Sep, 2012 3 commits
-
-
Daniel Vetter authored
This goes back to commit c1c7af60 Author: Jesse Barnes <jbarnes@virtuousgeek.org> Date: Thu Sep 10 15:28:03 2009 -0700 drm/i915: force mode set at lid open time It was used to fix an issue on a i915GM based Thinkpad X41, which somehow clobbered the modeset state at lid close time. Since then massive amounts of things changed: Tons of fixes to the modeset sequence, OpRegion support, better integration with the acpi code. Especially OpRegion /should/ allow us to control the display hw cooperatively with the firmware, without the firmware clobbering the hw state behind our backs. So it's dubious whether we still need this. The second issue is that it's unclear who's responsibility it actually is to restore the mode - Chris Wilson suggests to just emit a hotplug event and let userspace figure things out. The real reason I've stumbled over this is that the new modeset code breaks drm_helper_resume_force_mode - it OOPSes derefing a NULL vfunc pointer. The reason this wasn't caught in testing earlier is that in commit c9354c85 Author: Linus Torvalds <torvalds@linux-foundation.org> Date: Mon Nov 2 09:29:55 2009 -0800 i915: fix intel graphics suspend breakage due to resume/lid event confusion logic was added to _not_ restore the modeset state after a resume. And since most machines are configured to auto-suspend on lid-close, this neatly papered over the issue. Summarizing, this shouldn't be required on any platform supporting OpRegion. And none of the really old machines I have here seem to require it either. Hence I'm inclined to just rip it out. But in case that there are really firmwares out there that clobber the hw state, replace it with a call to intel_modset_check_state. This will ensure that we catch any issues as soon as they happen. Cc: Chris Wilson <chris@chris-wilson.co.uk> Acked-by:
Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by:
-
Daniel Vetter authored
While reworking the modeset sequence, this got lost in commit 25c5b266 Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Sun Jul 8 22:08:04 2012 +0200 drm/i915: implement new set_mode code flow I've noticed this because some Xorg versions seem to set up a new mode with every crtc at (0,0) and then pan to the right multi-monitor setup. And since some hacks of mine added more calls to mode_set using the stored crtc->x/y my multi-screen setup blew up. Reviewed-by:
-
Jerome Glisse authored
Limit printing bad edid information at one time per connector. Connector that are connected to a bad monitor/kvm will likely stay connected to the same bad monitor/kvm and it makes no sense to keep printing the bad edid message. Signed-off-by:
Jerome Glisse <jglisse@redhat.com> Reviewed-by:
Adam Jackson <ajax@redhat.com> Signed-off-by:
-
- 13 Sep, 2012 10 commits
-
-
Emil Goode authored
This patch removes a unused struct psb_intel_connector Sparse gives a warning: drivers/gpu/drm/gma500/cdv_intel_hdmi.c:142:30: warning: unused variable ‘psb_intel_connector’ [-Wunused-variable] Signed-off-by:
Emil Goode <emilgoode@gmail.com> Signed-off-by:
-
Wei Yongjun authored
The memory return by kzalloc() or kmem_cache_zalloc() has already be set to zero, so remove useless memset(0). Signed-off-by:
Wei Yongjun <yongjun_wei@trendmicro.com.cn> Signed-off-by:
-
Shirish S authored
The current logic for probing ddc is limited to 2 blocks (256 bytes), this patch adds support for the 4 block (512) data. To do this, a single 8-bit segment index is passed to the display via the I2C address 30h. Data from the selected segment is then immediately read via the regular DDC2 address using a repeated I2C 'START' signal. Signed-off-by:
Shirish S <s.shirish@samsung.com> Reviewed-by:
Jean Delvare <jdelvare@suse.de> Reviewed-by:
Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by:
-
Robert P. J. Day authored
Since arch/arm/include/asm/pgtable.h contains: #define io_remap_pfn_range(vma,from,pfn,size,prot) \ remap_pfn_range(vma, from, pfn, size, prot) there is no point treating ARM as a special case in distinguishing between remap_pfn_range() and io_remap_pfn_range(). Signed-off-by:Robert P. J. Day <rpjday@crashcourse.ca> Signed-off-by:
-
Andy Shevchenko authored
Signed-off-by:
Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by:
-
Ville Syrjälä authored
There are two slightly different pieces of code for HDMI VSDB detection. Unify the code into a single helper function. Also fix a bug where drm_detect_hdmi_monitor() would stop looking for the HDMI VSDB after the first vendor specific block is found, whether or not that block happened to be the HDMI VSDB. The standard allows for any number of vendor specific blocks to be present. Signed-off-by:
-
Ville Syrjälä authored
The length of HDMI VSDB must be at least 5 bytes. Other than the minimum, nothing else about the length is specified. Check the length before accessing any additional field beyond the minimum length. Signed-off-by:
-
Ville Syrjälä authored
Make sure drm_detect_hdmi_monitor() and drm_detect_monitor_audio() don't access beyond the extension block. Signed-off-by:
-
Russell King - ARM Linux authored
At the moment, there is an inconsistency in the way modes are named. Modes with timings parsed from the EDID information will call drm_mode_set_name(), which will name the mode using this form: <horizontal-res>x<vertical-res><interlace-char> eg, 1920x1080i for an interlaced mode, or 1920x1080 for a progressive mode. However, timings parsed using the tables in drm_edid_modes.h do not have the 'i' suffix. You are left to deduce that they're interlaced from xrandr's output by the lower vertical refresh frequencies. This patch changes the interlaced mode names in drm_edid_modes.h to follow the style set by drm_mode_set_name(), which makes it clear in xrandr which modes are interlaced and which are not (as xrandr groups the refresh rates on a line according to the name field.) Signed-off-by:
Russell King <rmk+kernel@arm.linux.org.uk> Reviewed-by:
Alex Deucher <alexander.deucher@amd.com> Reviewed-by:
-
David Herrmann authored
DRM users should be able to create/destroy/manage dumb- and frame-buffers without DRM_MASTER. These ioctls do not affect modesetting so there is no reason to protect them by drm-master. Particularly, destroying buffers should always be possible as a client has only access to buffers that they created. Hence, there is no reason to prevent a client from destroying the buffers, considering a simple close() would destroy them, anyway. Furthermore, a display-server currently cannot shutdown correctly if it does not have DRM_MASTER. If some other display-server becomes active (or the kernel console), then the background display-server is unable to destroy its buffers. Under special curcumstances (like monitor reconfiguration) this might even happen during runtime. Signed-off-by:
David Herrmann <dh.herrmann@googlemail.com> Signed-off-by:
-
- 07 Sep, 2012 1 commit
-
-
Daniel Vetter authored
Yet again a case where the fb helper is too intimate with the crtc helper and calls a crtc helepr function directly instead of going through the interface vtable. This fixes console blanking in drm/i915 with the new i915-specific modeset code. Reported-by:
Dave Airlie <airlied@gmail.com> Signed-off-by:
-
- 06 Sep, 2012 13 commits
-
-
Daniel Vetter authored
As a quick reference I'll detail the motivation and design of the new code a bit here (mostly stitched together from patchbomb announcements and commits introducing the new concepts). The crtc helper code has the fundamental assumption that encoders and crtcs can be enabled/disabled in any order, as long as we take care of depencies (which means that enabled encoders need an enabled crtc to feed them data, essentially). Our hw works differently. We already have tons of ugly cases where crtc code enables encoder hw (or encoder->mode_set enables stuff that should only be enabled in enocder->commit) to work around these issues. But on the disable side we can't pull off similar tricks - there we actually need to rework the modeset sequence that controls all this. And this is also the real motivation why I've finally undertaken this rewrite: eDP on my shiny new Ivybridge Ultrabook is broken, and it's broken due to the wrong disable sequence ... The new code introduces a few interfaces and concepts: - Add new encoder->enable/disable functions which are directly called from the crtc->enable/disable function. This ensures that the encoder's can be enabled/disabled at a very specific in the modeset sequence, controlled by our platform specific code (instead of the crtc helper code calling them at a time it deems convenient). - Rework the dpms code - our code has mostly 1:1 connector:encoder mappings and does support cloning on only a few encoders, so we can simplify things quite a bit. - Also only ever disable/enable the entire output pipeline. This ensures that we obey the right sequence of enabling/disabling things, trying to be clever here mostly just complicates the code and results in bugs. For cloneable encoders this requires a bit of special handling to ensure that outputs can still be disabled individually, but it simplifies the common case. - Add infrastructure to read out the current hw state. No amount of careful ordering will help us if we brick the hw on the initial modeset setup. Which could happen if we just randomly disable things, oblivious to the state set up by the bios. Hence we need to be able to read that out. As a benefit, we grow a few generic functions useful to cross-check our modeset code with actual hw state. With all this in place, we can copy&paste the crtc helper code into the drm/i915 driver and start to rework it: - As detailed above, the new code only disables/enables an entire output pipe. As a preparation for global mode-changes (e.g. reassigning shared resources) it keeps track of which pipes need to be touched by a set of bitmasks. - To ensure that we correctly disable the current display pipes, we need to know the currently active connector/encoder/crtc linking. The old crtc helper simply overwrote these links with the new setup, the new code stages the new links in ->new_* pointers. Those get commited to the real linking pointers once the old output configuration has been torn down, before the ->mode_set callbacks are called. - Finally the code adds tons of self-consistency checks by employing the new hw state readout functions to cross-check the actual hw state with what the datastructure think it should be. These checks are done both after every modeset and after the hw state has been read out and sanitized at boot/resume time. All these checks greatly helped in tracking down regressions and bugs in the new code. With this new basis, a lot of cleanups and improvements to the code are now possible (besides the DP fixes that ultimately made me write this), but not yet done: - I think we should create struct intel_mode and use it as the adjusted mode everywhere to store little pieces like needs_tvclock, pipe dithering values or dp link parameters. That would still be a layering violation, but at least we wouldn't need to recompute these kinds of things in intel_display.c. Especially the port bpc computation needed for selecting the pipe bpc and dithering settings in intel_display.c is rather gross. - In a related rework we could implement ->mode_valid in terms of ->mode_fixup in a generic way - I've hunted down too many bugs where ->mode_valid did the right thing, but ->mode_fixup didn't. Or vice versa, resulting in funny bugs for user-supplied modes. - Ditch the idea to rework the hdp handling in the common crtc helper code and just move things to i915.ko. Which would rid us of the ->detect crtc helper dependencies. - LVDS wire pair and pll enabling is all done in the crtc->mode_set function currently. We should be able to move this to the crtc_enable callbacks (or in the case of the LVDS wire pair enabling, into some encoder callback). Last, but not least, this new code should also help in enabling a few neat features: The hw state readout code prepares (but there are still big pieces missing) for fastboot, i.e. avoiding the inital modeset at boot-up and just taking over the configuration left behind by the bios. We also should be able to extend the configuration checks in the beginning of the modeset sequence and make better decisions about shared resources (which is the entire point behind the atomic/global modeset ioctl). Tested-by:
Jani Nikula <jani.nikula@intel.com> Tested-by:
Damien Lespiau <damien.lespiau@intel.com> Tested-by:
Rodrigo Vivi <rodrigo.vivi@gmail.com> Acked-by:
Vijay Purushothaman <vijay.a.purushothaman@intel.com> Acked-by:
Paulo Zanoni <paulo.r.zanoni@intel.com> Acked-by:
-
Daniel Vetter authored
Now that we have solid modeset state tracking and checking code in place, we can do the Full Monty also after dpms calls. Reviewed-by:
-
Daniel Vetter authored
... let's see whether this catches anything earlier and I can track down a few bugs. v2: Add more checks and also add DRM_DEBUG_KMS output so that it's clear which connector/encoder/crtc is being checked atm. Which proved rather useful for debugging ... v3: Add a WARN in the common encoder dpms function, now that also modeset changes properly update the dpms state ... v4: Properly add a short explanation for each WARN, to avoid the need to correlate dmesg lines with source lines accurately. Suggested by Chris Wilson. v5: Also dump (expected, found) for state checks (or wherever it's not apparent from the test what exactly mismatches with expectations). Again suggested by Chris Wilson. v6: Due to an issue reported by Paulo Zanoni I've noticed that the encoder checking is by far not as strict as it could and should be. Improve this. Reviewed-by:
-
Daniel Vetter authored
Since this only calls crtc helper functions, of which a shocking amount are NULL. Now the curious thing is how the new modeset code worked with this function call still present: Thanks to the hw state readout and the suspend fixes to properly quiescent the register state, nothing is actually enabled at resume (if the bios doesn't set up anything). Which means resume_force_mode doesn't actually do anything and hence nothing blows up at resume time. The other reason things do work is that the fbcon layer has it's own resume notifier callback, which restores the mode. And thanks to the force vt switch at suspend/resume, that then forces X to restore it's own mode. Hence everything still worked (as long as the bios doesn't enable anything). And we can just kill the call to resume_force_mode. The upside of both this patch and the preceeding patch to quiescent the modeset state is that our resume path is much simpler: - We now longer restore bogus register values (which most often would enable the backlight a bit and a few ports), causing flickering. - We now longer call resume_force_mode to restore a mode that the fbcon layer would overwrite right away anyway. Signed-off-by: -
Daniel Vetter authored
We need this to avoid confusing the hw state readout code with the cpt pch plls at resume time: We'd read the new pipe state (which is disabled), but still believe that we have a life pll connected to that pipe (from before the suspend). Hence properly disable pipes to clear out all the residual state. This has the neat side-effect that we don't enable ports prematurely by restoring bogus state from the saved register values. Reviewed-by:
-
Daniel Vetter authored
With this change we can (finally!) rip out a few of the temporary hacks and clean up a few other things: - Kill intel_crtc_prepare_encoders, now unused. - Kill the hacks in the crtc_disable/enable functions to always call the encoder callbacks, we now always call the crtc functions with the right encoder -> crtc links. - Also push down the crtc->enable, encoder and connector dpms state updates. Unfortunately we can't add a WARN in the crtc_disable callbacks to ensure that the crtc is always still enabled when disabling an output pipe - the crtc sanitizer of the hw readout path can hit this when it needs to disable an active pipe without any enabled outputs. - Only call crtc->disable if the pipe is already enabled - again avoids running afoul of the new WARN. v2: Copy&paste our own version of crtc_in_use, too. v3: We need to update the dpms an encoder->connectors_active states, too. v4: I've forgotten to kill the unconditional encoder->disable calls in the crtc_disable functions. v5: Rip out leftover debug printk. v6: Properly clear intel_encoder->connectors_active. This wasn't properly cleared when disabling an encoder because it was no longer on the new connector list, but the crtc was still enabled (i.e. switching the encoder of an active crtc). Reported by Jani Nikula. v7: Don't clobber the encoder->connectors_active state of untouched encoders. Since X likes to first disable all outputs with dpms off before setting a new framebuffer, this hit a few warnings. Reported by Paulo Zanoni. v8: Kill the now stale comment warning that intel_crtc->active is not always updated at the right times. Signed-off-by: -
Daniel Vetter authored
Now that set_mode also disables crtcs and expects it's new configuration in the staged output links we need to adjust the load detect code a bit. Reviewed-by:
-
Daniel Vetter authored
This seems to be the symptom of a few neat bugs, hence be more obnoxious when this fails. Reviewed-by:
-
Daniel Vetter authored
Because that's what it is. Unfortunately we can't rip this out because the fb helper has an incetious relationship with the crtc helper - it likes to call disable_unused_functions, among other things. Reviewed-by:
-
Daniel Vetter authored
This requires a few changes - We still need a noop function for crtc->disable, becuase the fb helper is a bit too intimate with the crtc helper. - We need to clear crtc->fb ourselves in intel_crtc_disable now that we no longer rely on the helper's disable_unused_functions to do that. - We need to split out the sare update code, becuase the crtc code can't call update_dpms any more, it needs to disable the crtc unconditionally. This is because we now keep onto the encoder -> crtc mapping of the (still) active output pipe configuration. - To check that we really disable a crtc that still has encoders, insert a WARN_ON(!enabled) in the crtc disable function. - Lastly, we need to walk over all crtcs to update their enabled state after having called commit_output_state - for all disabled crtcs the crtc helper code has done that for us previously. v2: Update connector dpms and encoder->connectors_active after disabling the crtc, too. v3: Noop-out intel_encoder_disable. Similarly to the crtc disable callback used by the crtc helper code we can't simply remove all these encoder callbacks: The fb helper (which we still use) has a rather incetious relationship with the crtc helper code ... Reviewed-by:
-
Daniel Vetter authored
... using the pipe masks from the previous patch. Well, not quite: - We still need to call the disable_unused_functions helper, until we've moved the call to commit_output_state further down and adjusted intel_crtc_disable a bit. The next patch will do that. - Because we don't support (yet) mode changes on more than one crtc at a time, some of the modeset_pipes checks are a bit hackish - but that only needs fixing once we incorporate global modeset support. Reviewed-by:
-
Daniel Vetter authored
This is definetely a bit more generic than currently required, but if we keep track of all crtcs that need to be disabled/enable (because they loose an encoder or something similar), crtcs that get completely disabled and those that we need to do an actual mode change nicely prepares us for global modeset operations on multiple crtcs. The only big thing missing here would be a global resource allocation step (for e.g. pch plls), which would equally frob these bitmasks if e.g. a crtc only needs a new pll. Or if we need to enable dithering on an another pipe due to bandwidth constrains somewhere. These masks aren't yet put to use in this patch, this will follow in the next one. v2-v5: Fix up the computations for good (hopefully). v6: Fixup a confusion reported by Damien Lespiau: I've conserved the (imo braindead) behaviour of the crtc helper to disable _any_ disconnected outputs if we do a modeset, even when that newly disabled connector isn't connected to the crtc being changed by the modeset. The effect of that is that we could disable an arbitrary number of unrelated crtcs, which I haven't taken into account when writing this code. Fix this up. Reviewed-by:
-
Daniel Vetter authored
- Use the check_cloned helper from the previous patch. - Use encoder->new_crtc to check crtc properties. v2: Kill the double negation with s/!non_cloned/is_cloned, suggested by Jesse Barnes. Reviewed-by:
-
