Hi Christian,
On Wed, 21 Jun 2023 15:36:59 +0200
"Christian König" <ckoenig.leichtzumerken@xxxxxxxxx> wrote:
> This adds the infrastructure for an execution context for GEM buffers
> which is similar to the existing TTMs execbuf util and intended to replace
> it in the long term.
>
> The basic functionality is that we abstracts the necessary loop to lock
> many different GEM buffers with automated deadlock and duplicate handling.
>
> v2: drop xarray and use dynamic resized array instead, the locking
> overhead is unecessary and measurable.
> v3: drop duplicate tracking, radeon is really the only one needing that.
> v4: fixes issues pointed out by Danilo, some typos in comments and a
> helper for lock arrays of GEM objects.
> v5: some suggestions by Boris Brezillon, especially just use one retry
> macro, drop loop in prepare_array, use flags instead of bool
One minor comment below, but otherwise, I think I'm happy with this version.
Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> +
> +/**
> + * drm_exec_prepare_array - helper to prepare an array of objects
> + * @exec: the drm_exec object with the state
> + * @objects: array of GEM object to prepare
> + * @num_objects: number of GEM objects in the array
> + * @num_fences: number of fences to reserve on each GEM object
> + *
> + * Prepares all GEM objects in an array, handles contention but aports on first
^
aborts
> + * error otherwise. Reserves @num_fences on each GEM object after locking it.
Either the documentation if wrong, or you unintentionally picked my
version. If that's the intended usage:
drm_exec_until_all_locked(exec) {
ret = drm_exec_prepare_array(exec, bos, num_bos, num_fences);
drm_exec_retry_on_contention(exec)
if (ret)
break;
}
you should drop the 'handles contention' part in the doc, and you
should probably give an example to show how it's supposed to be used.
> + *
> + * Returns: -EALREADY when object is already locked, -ENOMEM when memory
> + * allocation failed and zero for success.
> + */
> +int drm_exec_prepare_array(struct drm_exec *exec,
> + struct drm_gem_object **objects,
> + unsigned int num_objects,
> + unsigned int num_fences)
> +{
> + int ret;
> +
> + for (unsigned int i = 0; i < num_objects; ++i) {
> + ret = drm_exec_prepare_obj(exec, objects[i], num_fences);
> + if (unlikely(ret))
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_exec_prepare_array);
[...]
> +/**
> + * drm_exec_until_all_locked - loop until all GEM objects are locked
> + * @exec: drm_exec object
> + *
> + * Core functionality of the drm_exec object. Loops until all GEM objects are
> + * locked and no more contention exists. At the beginning of the loop it is
> + * guaranteed that no GEM object is locked.
> + *
> + * Since labels can't be defined local to the loops body we use a jump pointer
> + * to make sure that the retry is only used from within the loops body.
> + */
> +#define drm_exec_until_all_locked(exec) \
> + for (void *__drm_exec_retry_ptr; ({ \
> + __label__ __drm_exec_retry; \
> +__drm_exec_retry: \
> + __drm_exec_retry_ptr = &&__drm_exec_retry; \
> + drm_exec_cleanup(exec); \
> + });)
> +
> +/**
> + * drm_exec_retry_on_contention - restart the loop to grap all locks
> + * @exec: drm_exec object
> + *
> + * Control flow helper to continue when a contention was detected and we need to
> + * clean up and re-start the loop to prepare all GEM objects.
> + */
> +#define drm_exec_retry_on_contention(exec) \
> + if (unlikely(drm_exec_is_contended(exec))) \
> + goto *__drm_exec_retry_ptr
Glad that this ended up working.
Regards,
Boris
