Re: [PATCH 1/8] drm: execution context for GEM buffers v2

[Felix Kuehling <felix.kuehling@amd.com>]

Am 2022-06-09 um 03:18 schrieb Christian König:
> Am 09.06.22 um 02:05 schrieb Felix Kuehling:
>> [SNIP]
>>> + *
>>> + *        ret = drm_exec_lock(&exec, boB, 1);
>>
>> Where is drm_exec_lock? It's not in this patch.
>
> I've renamed this function to drm_exec_prepare_obj() but forgot to 
> update the documentation. Going to fix this, thanks.
>
>>
>>
>>> + * drm_exec_continue_on_contention(&exec);
>>> + *        if (ret)
>>> + *            goto error;
>>> + *    }
>>> + *
>>> + *    drm_exec_for_each_locked_object(&exec, index, obj) {
>>> + *        dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ);
>>> + *        ...
>>> + *    }
>>> + *    drm_exec_fini(&exec);
>>> + *
>>> + * See struct dma_exec for more details.
>>> + */
>>> +
>>> +/* Dummy value used to initially enter the retry loop */
>>> +#define DRM_EXEC_DUMMY (void*)~0
>>> +
>>> +/* Initialize the drm_exec_objects container */
>>> +static void drm_exec_objects_init(struct drm_exec_objects *container)
>>> +{
>>> +    container->objects = kmalloc(PAGE_SIZE, GFP_KERNEL);
>>
>> Should this be kvmalloc? You're using kvrealloc and kvfree elsewhere.
>
> The initial number of objects tracked should be small enough so that 
> we can use kmalloc() directly.

Ok. It just seems weird to allocate with kmalloc and then kvrealloc the 
same pointer. But if that's safe it doesn't matter.

BTW, if the caller already knows the number of BOs it wants to add, 
would is make sense to add that as a parameter to drm_exec_objects_init 
to save you all the reallocs?


>
>> [SNIP]
>>>
>>> +/**
>>> + * drm_exec_prepare_obj - prepare a GEM object for use
>>> + * @exec: the drm_exec object with the state
>>> + * @obj: the GEM object to prepare
>>> + * @num_fences: how many fences to reserve
>>> + *
>>> + * Prepare a GEM object for use by locking it and reserving fence 
>>> slots. All
>>> + * succesfully locked objects are put into the locked container. 
>>> Duplicates
>>> + * detected as well and automatically moved into the duplicates 
>>> container.
>>> + *
>>> + * Returns: -EDEADLK if a contention is detected, -ENOMEM when memory
>>> + * allocation failed and zero for success.
>>> + */
>>> +int drm_exec_prepare_obj(struct drm_exec *exec, struct 
>>> drm_gem_object *obj,
>>> +             unsigned int num_fences)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = drm_exec_lock_contended(exec);
>>
>> If this succeeds, it won't reserve any fence slots for object. Is 
>> that a problem?
>
> No, the contended object should be re-presented later on and then we 
> allocate the fence slots.

Ok. Along with all other objects, because drm_exec_cleanup will have 
cleaned up the lists.


>>>
>>> +    if (unlikely(ret == -EALREADY)) {
>>> +        struct drm_exec_objects *container = &exec->duplicates;
>>> +
>>> +        /*
>>> +         * If this is the first locked GEM object it was most likely
>>> +         * just contended. So don't add it to the duplicates, just
>>> +         * reserve the fence slots.
>>
>> I don't understand this. Seems a bit arbitrary. Is it even legal to 
>> try to add the same object twice? I thought duplicates was for 
>> different objects that share the same reservation, not actually the 
>> same object on the same list twice.
>
> Yes, it's legal to try the same object twice. That can easily happen 
> when userspace specifies the same handle twice for a submission.
>
> The other possibility is that we had a contention, backed off and then 
> locked the contention first and then re-tried everything else once more.
>
>> Maybe you meant to compare with 
>> exec->locked.objects[exec->locked.num_objects-1], assuming that 
>> drm_exec_lock_contended just succeeded locking a previously contended 
>> object, and the caller retried locking that same object again?
>
> Yes, that's pretty much the idea. But then exec->locked.num_objects is 
> just 1 so that's equal to checking exec->locked.num_objects[0].

Ok. I missed that after drm_exec_continue_on_contention the 
drm_exec_cleanup will clean up the lists and we start over locking all 
the objects. Why can't drm_exec_cleanup just lock the contended object 
itself? I think that would make it more obvious that the contended 
object is always the first one that gets locked and added to the list.

Thanks,
   Felix


>
> On the other hand we would also catch cases where we lock the same 
> object multiple times directly behind each other. Need to think about 
> that a bit.
>
>>> +         */
>>> +        if (exec->locked.num_objects && exec->locked.objects[0] == 
>>> obj)
>>> +            container = NULL;
>>> +
>>> +        ret = drm_exec_obj_locked(container, obj, num_fences);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +    } else if (unlikely(ret)) {
>>> +        return ret;
>>> +
>>> +    } else {
>>> +        ret = drm_exec_obj_locked(&exec->locked, obj, num_fences);
>>> +        if (ret)
>>> +            goto error_unlock;
>>> +    }
>>> +
>>> +    drm_gem_object_get(obj);
>>
>> The container already gets a reference to obj. What is this extra 
>> reference for? And where does it get dropped?
>
> Need to double check this, might be that it's just a leftover.
>
> Thanks,
> Christian.
>
>>
>> Regards,
>>   Felix
>