[Mesa-dev] [PATCH] gallium/sw/kms: Fix multiple imports from PRIME FDs

[Tomasz Figa]

Hi Emil,

On Sat, Jul 16, 2016 at 6:38 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 15 July 2016 at 08:27, Tomasz Figa <tfiga at chromium.org> wrote:
>> When a buffer with a GEM handle already existing in our context is
>> (re-)imported from a PRIME FD, the resulting GEM handle is exactly the
>> same as the original one. Since the GEM handles are not reference
>> counted, we need to detect duplicate imports and reference count our
>> internal buffer structs ourselves.
>>
>> Signed-off-by: Tomasz Figa <tfiga at chromium.org>
>> ---
>>  src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c | 30 +++++++++++++++++------
>>  1 file changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> index 21ac0d7..c4f56cb 100644
>> --- a/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> +++ b/src/gallium/winsys/sw/kms-dri/kms_dri_sw_winsys.c
>> @@ -211,7 +211,9 @@ kms_sw_displaytarget_map(struct sw_winsys *ws,
>>  }
>>
>>  static struct kms_sw_displaytarget *
>> -kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd)
>> +kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd,
>> +                                    unsigned width, unsigned height,
>> +                                    unsigned stride)
>>  {
>>     uint32_t handle = -1;
>>     struct kms_sw_displaytarget * kms_sw_dt;
>> @@ -222,6 +224,17 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd)
>>     if (ret)
>>        return NULL;
>>
>> +   LIST_FOR_EACH_ENTRY(kms_sw_dt, &kms_sw->bo_list, link) {
>> +      if (kms_sw_dt->handle == handle) {
>> +         kms_sw_dt->ref_count++;
>> +
>> +         DEBUG_PRINT("KMS-DEBUG: imported buffer %u (size %u)\n",
>> +                     kms_sw_dt->handle, kms_sw_dt->size);
>> +
>> +         return kms_sw_dt;
>> +      }
>> +   }
>> +
>>     kms_sw_dt = CALLOC_STRUCT(kms_sw_displaytarget);
>>     if (!kms_sw_dt)
>>        return NULL;
>> @@ -229,6 +242,9 @@ kms_sw_displaytarget_add_from_prime(struct kms_sw_winsys *kms_sw, int fd)
>>     kms_sw_dt->ref_count = 1;
>>     kms_sw_dt->handle = handle;
>>     kms_sw_dt->size = lseek(fd, 0, SEEK_END);
>> +   kms_sw_dt->width = width;
>> +   kms_sw_dt->height = height;
>> +   kms_sw_dt->stride = stride;
>>
>>     if (kms_sw_dt->size == (off_t)-1) {
>>        FREE(kms_sw_dt);
>> @@ -274,14 +290,12 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
>>
>>     switch(whandle->type) {
>>     case DRM_API_HANDLE_TYPE_FD:
>> -      kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle);
>> -      if (kms_sw_dt) {
>> -         kms_sw_dt->ref_count++;
>> -         kms_sw_dt->width = templ->width0;
>> -         kms_sw_dt->height = templ->height0;
>> -         kms_sw_dt->stride = whandle->stride;
>> +      kms_sw_dt = kms_sw_displaytarget_add_from_prime(kms_sw, whandle->handle,
>> +                                                      templ->width0,
>> +                                                      templ->height0,
>> +                                                      whandle->stride);
>> +      if (kms_sw_dt)
>>           *stride = kms_sw_dt->stride;
>> -      }
> So there's actually a couple of things here:
>  - kms_sw_displaytarget_add_from_prime now takes a width, height,
> stride. not a bugfix but very nice imho.
>  - DRM_API_HANDLE_TYPE_FD were ref_counted twice upon creation - once
> in kms_sw_displaytarget_add_from_prime, and a second time immediatelly
> after.
>  - one should have checked the bo_list prior to importing the BO as prime FD.
>
> Can we split the former (2/2) and get the other two (1/2) with CC:
> mesa-stable... ?
> Ideally we would have just a single
> "LIST_FOR_EACH_ENTRY(...&kms_sw->bo_list...)" block in
> kms_sw_displaytarget_from_handle and none in
> kms_sw_displaytarget_add_from_prime.

It's not quite obvious to me how could I split 1) from 3) in a way
that 3) would be before 1). It's because after the look up, if and
only if no match was found, we need to create a new display target and
fill in the struct. If we got a match from the lookup, we should
return it without changing contents of the struct, other than
refcount.

Considering the above, would the following split be okay for you?

1) Fix the double refcount (second point of your list).
2) Move struct initialization to kms_sw_displaytarget_add_from_prime()
(first point of your list).
3) Move KMS (or rather GEM) handle lookup (with refcount++) to a
separate helper function and call it in
kms_sw_displaytarget_add_from_prime() after importing the FD.

All of the above would be CCed to mesa-stable. What do you think?

Best regards,
Tomasz