Hi Chris,
On Wed, May 29, 2019 at 02:24:21PM +0100, Chris Wilson wrote:
> If we run out of engines, intel_get_current_physical_engine() degrades
> into an infinite loop as although it advanced the iterator, it did not
> update its local engine pointer.
The patch looks like it does everything "but" what you say in the
commit log :)
>
> Reported-by: Petri Latvala <petri.latvala@xxxxxxxxx>
> Fixes: 17c77e7b0c3c ("lib/i915: add gem_engine_topology library and for_each loop definition")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Andi Shyti <andi.shyti@xxxxxxxxx>
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> ---
> lib/i915/gem_engine_topology.c | 49 +++++-----------------------------
> lib/i915/gem_engine_topology.h | 36 ++++++++++++++++---------
> 2 files changed, 29 insertions(+), 56 deletions(-)
>
> diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
> index fdd1b9516..17f67786f 100644
> --- a/lib/i915/gem_engine_topology.c
> +++ b/lib/i915/gem_engine_topology.c
> @@ -81,11 +81,10 @@ static void ctx_map_engines(int fd, struct intel_engine_data *ed,
> struct drm_i915_gem_context_param *param)
> {
> struct i915_context_param_engines *engines =
> - from_user_pointer(param->value);
> + from_user_pointer(param->value);
> int i = 0;
>
> - for (typeof(engines->engines[0]) *p =
> - &engines->engines[0];
> + for (struct i915_engine_class_instance *p = &engines->engines[0];
> i < ed->nengines; i++, p++) {
> p->engine_class = ed->engines[i].class;
> p->engine_instance = ed->engines[i].instance;
> @@ -136,7 +135,7 @@ static void query_engine_list(int fd, struct intel_engine_data *ed)
> {
> uint8_t buff[SIZEOF_QUERY] = { };
> struct drm_i915_query_engine_info *query_engine =
> - (struct drm_i915_query_engine_info *) buff;
> + (struct drm_i915_query_engine_info *)buff;
Until here, nothing is related to the description in the commit
log. Can we put the above in a different patch?
> -struct intel_execution_engine2 *
> -intel_get_current_engine(struct intel_engine_data *ed)
> -{
> - if (!ed->n)
> - ed->current_engine = &ed->engines[0];
> - else if (ed->n >= ed->nengines)
> - ed->current_engine = NULL;
> -
> - return ed->current_engine;
> -}
> -
> -void intel_next_engine(struct intel_engine_data *ed)
> -{
> - if (ed->n + 1 < ed->nengines) {
> - ed->n++;
> - ed->current_engine = &ed->engines[ed->n];
> - } else {
> - ed->n = ed->nengines;
> - ed->current_engine = NULL;
> - }
> -}
> -
> -struct intel_execution_engine2 *
> -intel_get_current_physical_engine(struct intel_engine_data *ed)
> -{
> - struct intel_execution_engine2 *e;
> -
> - for (e = intel_get_current_engine(ed);
> - e && e->is_virtual;
> - intel_next_engine(ed))
> - ;
> -
> - return e;
> -}
> -
Moving these functions to inline in the header file is unrelated
to the patch topic, right?
> static int gem_topology_get_param(int fd,
> struct drm_i915_gem_context_param *p)
> {
> @@ -197,10 +161,9 @@ static int gem_topology_get_param(int fd,
> return 0;
>
> /* size will store the engine count */
> - p->size = (p->size - sizeof(struct i915_context_param_engines)) /
> - (offsetof(struct i915_context_param_engines,
> - engines[1]) -
> - sizeof(struct i915_context_param_engines));
> + igt_assert(p->size >= sizeof(struct i915_context_param_engines));
> + p->size -= sizeof(struct i915_context_param_engines);
> + p->size /= sizeof(struct i915_engine_class_instance);
This is also unrelated.
> struct intel_engine_data {
> - uint32_t nengines;
> - uint32_t n;
> - struct intel_execution_engine2 *current_engine;
so we don't have anymore current_engine... I had the feeling the
Tvrtko really wanted it :)
> + uint32_t nengines, cur;
[ some copy paste ]
> - uint32_t nengines;
> - uint32_t n;
> + uint32_t nengines, cur;
mmhhh... why?
> +static inline struct intel_execution_engine2 *
> +intel_get_current_engine(struct intel_engine_data *ed)
> +{
> + if (ed->cur >= ed->nengines)
> + return NULL;
> +
> + return &ed->engines[ed->cur];
> +}
> +
> +static inline struct intel_execution_engine2 *
> +intel_get_current_physical_engine(struct intel_engine_data *ed)
> +{
> + struct intel_execution_engine2 *e;
> +
> + for (; (e = intel_get_current_engine(ed)) && e->is_virtual; ed->cur++)
> + ;
The above two lines are the only ones related to the commit
message. Can we keep this patch smaller? and put cosmetics in a
different patchset?
Thanks,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
