[Mesa-dev] [PATCH] kms-swrast: Support Prime fd handling

[Giovanni Campagna]

On Thu, Aug 28, 2014 at 4:01 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 22/08/14 17:41, Andreas Pokorny wrote:
>> Allows using prime fds as display target and from display target.
>> Test for PRIME capability after initializing kms_swrast screen.
>>
> Hi Andreas,
>
> I'm hoping that Giovanni will take a look. After all kms-dri is his creation.

I'm not sure I'm the right person to look at this code. After all I'm
not a mesa developer.

>From a cursory review, the patches look good to me, but what I'm not
seeing are the necessary EGL changes to make use of prime fds with
kms-swrast, eg. EGL_WL_bind_wayland_display is still not exposed, even
though it could be implemented using prime fds.
Another thing is, you briefly mentioned the generic GEM ioctls. If
drivers really can use the generic GEM_FLINK/GEM_OPEN ioctl to give a
name to dumb buffers, then indeed the kms-swrast driver is capable of
buffer sharing (I always assumed that a somehow real GEM was
necessary, and that the simple one in qxl/cirrus/simpledrm was not
enough). I'd like to see that implemented first, which would allow to
remove a good amount of complexity and special casing in gallium and
egl (and would allow using kms-swrast for faster sw wayland too).

Giovanni

> From my POV the patch is good and should be safe to go in 10.3 on the point
> that the driver/target was introduced with 10.3. There is a trivial nit-pick
> which I'll squash before committing later on this week.
>
> Btw in the future can you please include Cc: "branch number" <mesa-stable...>
> [1] in the commit message - it will ease things a bit :)
>
> Cheers,
> Emil
>
> [1] Marking a commit as a candidate for a stable branch
> http://mesa3d.org/devinfo.html
>
>> Signed-off-by: Andreas Pokorny <andreas.pokorny at canonical.com>
>> ---
> [snip]
>> 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 c9934bb..ba0660c 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
> [snip]
>> @@ -231,17 +264,34 @@ kms_sw_displaytarget_from_handle(struct sw_winsys *ws,
>>     struct kms_sw_winsys *kms_sw = kms_sw_winsys(ws);
>>     struct kms_sw_displaytarget *kms_sw_dt;
>>
>> -   assert(whandle->type == DRM_API_HANDLE_TYPE_KMS);
>> +   assert(whandle->type == DRM_API_HANDLE_TYPE_KMS ||
>> +          whandle->type == DRM_API_HANDLE_TYPE_FD);
>>
>> -   LIST_FOR_EACH_ENTRY(kms_sw_dt, &kms_sw->bo_list, link) {
>> -      if (kms_sw_dt->handle == whandle->handle) {
>> +   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;
>> +         if (stride)
> Any objections on dropping the above conditional before committing?
>
>> +            *stride = kms_sw_dt->stride;
>> +      }
>> +      return (struct sw_displaytarget *)kms_sw_dt;
>> +   case DRM_API_HANDLE_TYPE_KMS:
>> +      LIST_FOR_EACH_ENTRY(kms_sw_dt, &kms_sw->bo_list, link) {
>> +         if (kms_sw_dt->handle == whandle->handle) {
>> +            kms_sw_dt->ref_count++;
>>
>> -         DEBUG("KMS-DEBUG: imported buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
>> +            DEBUG("KMS-DEBUG: imported buffer %u (size %u)\n", kms_sw_dt->handle, kms_sw_dt->size);
>>
>> -         *stride = kms_sw_dt->stride;
>> -         return (struct sw_displaytarget *)kms_sw_dt;
>> +            *stride = kms_sw_dt->stride;
>> +            return (struct sw_displaytarget *)kms_sw_dt;
>> +         }
>>        }
>> +   default:
>> +   break;
>>     }