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

[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?

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
>>>
>