On 11/01/16 09:17, Chris Wilson wrote:
Both perform the same actions with more or less indirection, so just
unify the code.
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/i915_gem_context.c | 8 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 ++++-----
drivers/gpu/drm/i915/i915_gem_gtt.c | 26 +++----
drivers/gpu/drm/i915/intel_display.c | 26 +++----
drivers/gpu/drm/i915/intel_lrc.c | 114 ++++++++++++++---------------
drivers/gpu/drm/i915/intel_lrc.h | 26 -------
drivers/gpu/drm/i915/intel_mocs.c | 30 ++++----
drivers/gpu/drm/i915/intel_overlay.c | 42 +++++------
drivers/gpu/drm/i915/intel_ringbuffer.c | 101 ++++++++++++-------------
drivers/gpu/drm/i915/intel_ringbuffer.h | 21 ++----
11 files changed, 194 insertions(+), 236 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c2a1ec8abc11..247731672cb1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4068,7 +4068,7 @@ err:
int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
{
- struct intel_engine_cs *ring = req->ring;
+ struct intel_ringbuffer *ring = req->ringbuf;
NAK. (regardless of the fact that I'd like them unified!)
Until you have purged the last use of the name "ring" as a reference to
an engine, adding new things called "ring" but of a different type will
be too confusing.
The variable, at least for now, should be called "ringbuf", which makes
it obvious that it caches the 'req->ringbuf' field and NOT the
struct intel_engine_cs *ring;
that is also found in struct drm_i915_gem_request.
You can only start reusing names with a new meaning /after/ the old
meaning has been eliminated from the code, but some interval for
everyone's mental cache to be updated. But probably better never to
reuse an old name for a different thing, why not just make up a new one,
as we did with "engine".
struct drm_i915_private *dev_priv = req->i915;
u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
int i, ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3e3b4bf3fed1..d58de7e084dc 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -519,7 +519,7 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
static inline int
mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
{
- struct intel_engine_cs *ring = req->ring;
+ struct intel_ringbuffer *ring = req->ringbuf;
u32 flags = hw_flags | MI_MM_SPACE_GTT;
const int num_rings =
/* Use an extended w/a on ivb+ if signalling from other rings */
@@ -534,7 +534,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
* itlb_before_ctx_switch.
*/
if (IS_GEN6(req->i915)) {
- ret = ring->flush(req, I915_GEM_GPU_DOMAINS, 0);
+ ret = req->ring->flush(req, I915_GEM_GPU_DOMAINS, 0);
Hmm ... what is this "ring"? Oh, this one's not a ringbuffer, it's an
ENGINE!
if (ret)
return ret;
}
@@ -562,7 +562,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
for_each_ring(signaller, req->i915, i) {
- if (signaller == ring)
+ if (signaller == req->ring)
another engine
continue;
intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
@@ -587,7 +587,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
for_each_ring(signaller, req->i915, i) {
- if (signaller == ring)
+ if (signaller == req->ring)
and this one too
continue;
intel_ring_emit_reg(ring, RING_PSMI_CTL(signaller->mmio_base));
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 78b462956c78..603a247ac333 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1146,14 +1146,12 @@ i915_gem_execbuffer_retire_commands(struct i915_execbuffer_params *params)
}
static int
-i915_reset_gen7_sol_offsets(struct drm_device *dev,
- struct drm_i915_gem_request *req)
+i915_reset_gen7_sol_offsets(struct drm_i915_gem_request *req)
{
- struct intel_engine_cs *ring = req->ring;
- struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_ringbuffer *ring = req->ringbuf;
But this 'ring' is a ringbuffer ...
int ret, i;
- if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS]) {
+ if (!IS_GEN7(req->i915) || req->ring->id != RCS) {
... and this one, in the same function, is an engine!
DRM_DEBUG("sol reset is gen7/rcs only\n");
return -EINVAL;
}
So please submit a version that does just what it says ('cos I think
unification would be good) but without the confusing repurposing of
local variable names.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
