On 27/07/16 16:28, Chris Wilson wrote:
On Wed, Jul 27, 2016 at 12:08:35PM +0100, Dave Gordon wrote:
On 25/07/16 10:18, Joonas Lahtinen wrote:
On ma, 2016-07-25 at 08:44 +0100, Chris Wilson wrote:
If is simpler and leads to more readable code through the callstack if
the allocation returns the allocated struct through the return value.
The importance of this is that it no longer looks like we accidentally
allocate requests as side-effect of calling .
Dave seems to have expressed to wish to review this around January, so
CC'ing him here.
>From me,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/i915_gem.c | 75 ++++++++----------------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++---
drivers/gpu/drm/i915/i915_gem_request.c | 58 ++++++++---------------
drivers/gpu/drm/i915/i915_trace.h | 13 +++---
drivers/gpu/drm/i915/intel_display.c | 36 ++++++--------
drivers/gpu/drm/i915/intel_lrc.c | 2 +-
drivers/gpu/drm/i915/intel_overlay.c | 20 ++++----
8 files changed, 79 insertions(+), 140 deletions(-)
The actual code looks fine, but the patch amalgamates several
different changes, only one of which is mentioned in the
description above.
1. Change the signature of i915_gem_object_sync() and its internal
subfunction __i915_gem_object_sync() to always require a request
as input, with corresponding rework of the callsites. This enables
the removal of the internal call to i915_gem_request_alloc().
2. Change API of i915_gem_request_alloc() not to allow NULL ctx,
which involves changing various of the remaining callsites.
Reverts the unwanted the change.
No, 2 and 3 together do NOT revert the change 268270883 because that's
what originally changed the API of i915_gem_request_alloc() to return a
pointer directly, rather than via an output parameter. Which everyone
agrees is a good idea :)
Allowing NULL here was a useful step in the elimination of the per-ring
default context (which was after all your idea, IIRC). Now that there
aren't so many callsites it's not so ugly to have those callers mention
the (now-unified) kernel context.
3. Flatten __i915_gem_request_alloc() into i915_gem_request_alloc().
The description really only covers (3), which is the simplest and
most mechanical of the changes. So preferably two or three separate
patches, in whatever order makes the most sense, with a bit more
description of each.
It mentions 1 explicitly which was the reason why the API was originally
intended to be this.
-Chris
Strange, I can't see any *explicit* mention of i915_gem_object_sync(),
only some vague allusion to "certain functions".
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
