[PATCH v2 1/4] drm/vc4: Use DRM Execution Contexts

[Christian König]

Am 17.12.24 um 12:09 schrieb Maíra Canal:
> Hi Christian,
>
> On 17/12/24 07:30, Christian König wrote:
>> Am 16.12.24 um 20:08 schrieb Melissa Wen:
>>> On 12/12, Maíra Canal wrote:
>>>> VC4 has internal copies of `drm_gem_lock_reservations()` and
>>>> `drm_gem_unlock_reservations()` inside the driver and ideally, we 
>>>> should
>>>> move those hard-coded functions to the generic functions provided 
>>>> by DRM.
>>>> But, instead of using the DRM GEM functions to (un)lock 
>>>> reservations, move
>>>> the new DRM Execution Contexts API.
>>>>
>>>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>>>> ---
>>>>   drivers/gpu/drm/vc4/Kconfig   |  1 +
>>>>   drivers/gpu/drm/vc4/vc4_gem.c | 99 
>>>> ++++++++---------------------------
>>>>   2 files changed, 22 insertions(+), 78 deletions(-)
>>>>
>
> [...]
>
>>>> -
>>>> -    ww_acquire_done(acquire_ctx);
>>>> +    int ret;
>>>>       /* Reserve space for our shared (read-only) fence references,
>>>>        * before we commit the CL to the hardware.
>>>>        */
>>>> -    for (i = 0; i < exec->bo_count; i++) {
>>>> -        bo = exec->bo[i];
>>>> +    drm_exec_init(exec_ctx, DRM_EXEC_INTERRUPTIBLE_WAIT, exec- 
>>>> >bo_count);
>>>> +    drm_exec_until_all_locked(exec_ctx) {
>>>> +        ret = drm_exec_prepare_array(exec_ctx, exec->bo,
>>>> +                         exec->bo_count, 1);
>>> Hi Maíra,
>>>
>>> So, I'm not familiar too drm_exec, but the original implementation of
>>> vc4_lock_bo_reservations() has a retry of locks from the contention and
>>> I don't see it inside the drm_exec_prepare_array(), why don't use the
>>> loop drm_exec_prepare_obj() plus drm_exec_retry_on_contention() 
>>> (similar
>>> to the typical usage documented for drm_exec)?
>>
>> The way how drm_exec and drm_exec_prepare_array is used seems to be 
>> correct here.
>>
>> drm_exec_prepare_array() basically just loops over all the GEM BOs in 
>> the array and calls drm_exec_prepare_obj() on them, so no need to 
>> open code that.
>>
>> drm_exec_retry_on_contention() is only needed if you have multiple 
>> calls to drm_exec_prepare_array() or drm_exec_prepare_obj(), so that 
>> the loop is restarted in between the calls.
>
> But doesn't `drm_exec_prepare_array()` have multiple calls to
> `drm_exec_prepare_obj()`? The fact that the multiple calls are wrapped
> in the function makes a difference?

Yeah. I know, it's not so easy to understand :)

What drm_exec_until_all_locked() and drm_exec_retry_on_contention() are 
basically doing is nicely wrapped error handling.

In other words drm_exec_retry_on_contention() does a "goto 
*__drm_exec_retry_ptr" if it detects that we have a contention. But you 
can't goto from a label in a function back into the caller.

So what drm_exec_prepare_array() does is to abort as soon as it sees the 
first error:

                 ret = drm_exec_prepare_obj(exec, objects[i], num_fences);
                 if (unlikely(ret))
                         return ret;


And in the caller we have:

drm_exec_until_all_locked(exec_ctx) {
     ret = drm_exec_prepare_array(exec_ctx, exec->bo, exec->bo_count, 1);
}

So the loop restarts after drm_exec_prepare_array() anyway and because 
of this using drm_exec_retry_on_contention() is not explicitly necessary.

Regards,
Christian.

>
> Best Regards,
> - Maíra
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Also, probably you already considered that: we can extend this 
>>> update to
>>> v3d, right?
>>>
>>> Melissa
>>>
>>>> +    }
>>>> -        ret = dma_resv_reserve_fences(bo->resv, 1);
>>>> -        if (ret) {
>>>> -            vc4_unlock_bo_reservations(dev, exec, acquire_ctx);
>>>> -            return ret;
>>>> -        }
>>>> +    if (ret) {
>>>> +        drm_exec_fini(exec_ctx);
>>>> +        return ret;
>>>>       }
>>>>       return 0;
>>>> @@ -679,7 +620,7 @@ vc4_lock_bo_reservations(struct drm_device *dev,
>>>>    */
>>>>   static int
>>>>   vc4_queue_submit(struct drm_device *dev, struct vc4_exec_info *exec,
>>>> -         struct ww_acquire_ctx *acquire_ctx,
>>>> +         struct drm_exec *exec_ctx,
>>>>            struct drm_syncobj *out_sync)
>>>>   {
>>>>       struct vc4_dev *vc4 = to_vc4_dev(dev);
>>>> @@ -708,7 +649,7 @@ vc4_queue_submit(struct drm_device *dev, struct 
>>>> vc4_exec_info *exec,
>>>>       vc4_update_bo_seqnos(exec, seqno);
>>>> -    vc4_unlock_bo_reservations(dev, exec, acquire_ctx);
>>>> +    drm_exec_fini(exec_ctx);
>>>>       list_add_tail(&exec->head, &vc4->bin_job_list);
>>>> @@ -1123,7 +1064,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, 
>>>> void *data,
>>>>       struct drm_vc4_submit_cl *args = data;
>>>>       struct drm_syncobj *out_sync = NULL;
>>>>       struct vc4_exec_info *exec;
>>>> -    struct ww_acquire_ctx acquire_ctx;
>>>> +    struct drm_exec exec_ctx;
>>>>       struct dma_fence *in_fence;
>>>>       int ret = 0;
>>>> @@ -1216,7 +1157,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, 
>>>> void *data,
>>>>       if (ret)
>>>>           goto fail;
>>>> -    ret = vc4_lock_bo_reservations(dev, exec, &acquire_ctx);
>>>> +    ret = vc4_lock_bo_reservations(exec, &exec_ctx);
>>>>       if (ret)
>>>>           goto fail;
>>>> @@ -1224,7 +1165,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, 
>>>> void *data,
>>>>           out_sync = drm_syncobj_find(file_priv, args->out_sync);
>>>>           if (!out_sync) {
>>>>               ret = -EINVAL;
>>>> -            goto fail;
>>>> +            goto fail_unreserve;
>>>>           }
>>>>           /* We replace the fence in out_sync in vc4_queue_submit 
>>>> since
>>>> @@ -1239,7 +1180,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, 
>>>> void *data,
>>>>        */
>>>>       exec->args = NULL;
>>>> -    ret = vc4_queue_submit(dev, exec, &acquire_ctx, out_sync);
>>>> +    ret = vc4_queue_submit(dev, exec, &exec_ctx, out_sync);
>>>>       /* The syncobj isn't part of the exec data and we need to 
>>>> free our
>>>>        * reference even if job submission failed.
>>>> @@ -1248,13 +1189,15 @@ vc4_submit_cl_ioctl(struct drm_device *dev, 
>>>> void *data,
>>>>           drm_syncobj_put(out_sync);
>>>>       if (ret)
>>>> -        goto fail;
>>>> +        goto fail_unreserve;
>>>>       /* Return the seqno for our job. */
>>>>       args->seqno = vc4->emit_seqno;
>>>>       return 0;
>>>> +fail_unreserve:
>>>> +    drm_exec_fini(&exec_ctx);
>>>>   fail:
>>>>       vc4_complete_exec(&vc4->base, exec);
>>>> -- 
>>>> 2.47.1
>>>>
>>
>