On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -155,10 +155,10 @@ describe_obj(struct seq_file *m, struct
> drm_i915_gem_object *obj)
> obj->base.write_domain);
> for_each_engine_id(engine, dev_priv, id)
> seq_printf(m, "%x ",
> -
> i915_gem_request_get_seqno(obj->last_read_req[id]));
> +
> i915_gem_request_get_seqno(obj->last_read[id].request));
I hate i915_gem_request_get_seqno already, it's just NULL protection,
but subject to different patch. Although, I see you got rid
of i915_gem_request_get_engine already.
> @@ -2383,10 +2383,10 @@ void i915_vma_move_to_active(struct i915_vma *vma,
> static void
> i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
> {
> - GEM_BUG_ON(obj->last_write_req == NULL);
> - GEM_BUG_ON(!(obj->active &
> intel_engine_flag(obj->last_write_req->engine)));
> + GEM_BUG_ON(!obj->last_write.request);
> + GEM_BUG_ON(!(obj->active &
> intel_engine_flag(obj->last_write.request->engine)));
Over 80 ch line.
intel_engine_flag seems rather dull, there's also this thing called
BIT(), but again subject to another series.
> @@ -2395,13 +2395,13 @@ i915_gem_object_retire__read(struct
> drm_i915_gem_object *obj, int idx)
> {
> struct i915_vma *vma;
>
> - GEM_BUG_ON(obj->last_read_req[idx] == NULL);
> + GEM_BUG_ON(!obj->last_read[idx].request);
> GEM_BUG_ON(!(obj->active & (1 << idx)));
BIT(idx)?
> +/* We treat requests as fences. This is not be to confused with our
> + * "fence registers" but pipeline synchronisation objects ala GL_ARB_sync.
> + * We use the fences to synchronize access from the CPU with activity on the
> + * GPU, for example, we should not rewrite an object's PTE whilst the GPU
> + * is reading them. We also track fences at a higher level to provide
> + * implicit synchronisation around GEM objects, e.g. set-domain will wait
> + * for outstanding GPU rendering before marking the object ready for CPU
> + * access, or a pageflip will wait until the GPU is complete before showing
> + * the frame on the scanout.
> + *
> + * In order to use a fence, the object must track the fence it needs to
> + * serialise with. For example, GEM objects want to track both read and
> + * write access so that we can perform concurrent read operations between
> + * the CPU and GPU engines, as well as waiting for all rendering to
> + * complete, or waiting for the last GPU user of a "fence register". The
> + * object then embeds a @i915_gem_active to track the most recent (in
> + * retirment order) request relevant for the desired mode of access.
> + * The @i915_gem_active is updated with i915_gem_request_mark_active() to
> + * track the most recent fence request, typically this is done as part of
> + * i915_vma_move_to_active().
> + *
> + * When the @i915_gem_active completes (is retired), it will
> + * signal its completion to the owner through a callback as well as mark
> + * itself as idle (i915_gem_active.request == NULL). The owner
> + * can then perform any action, such as delayed freeing of an active
> + * resource including itself.
> + */
> +struct i915_gem_active {
Not sure if this is a good descriptive struct name. Would not this be
in the sync terminology a fence? active.request reads nicely though.
> + struct drm_i915_gem_request *request;
> +};
> +
> +static inline void
> +i915_gem_active_set(struct i915_gem_active *active,
> + struct drm_i915_gem_request *request)
> +{
> + i915_gem_request_assign(&active->request, request);
> +}
> +
> +#define for_each_active(mask, idx) \
> + for (; mask ? idx = ffs(mask) - 1, 1 : 0; mask &= ~(1 << idx))
BIT()
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11378,7 +11378,7 @@ static bool use_mmio_flip(struct intel_engine_cs
> *engine,
> if (resv && !reservation_object_test_signaled_rcu(resv, false))
> return true;
>
> - return engine != i915_gem_request_get_engine(obj->last_write_req);
> + return engine != i915_gem_request_get_engine(obj->last_write.request);
What's been the obsession with NULL protecting simple accessor
functions? Makes the code look overly complicated. One more function to
nuke.
The changes itself make sense, with above tweaks;
Reviewed-by: Joonas Lahtinen <[email protected]>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
