[PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm

[Esaki Tomohito]

Hello Daniel-san

On 2017/09/27 2:09, Daniel Stone wrote:
> Hello Esaki-san,
> 
> On 1 November 2016 at 19:09, Daniel Stone <daniel at fooishbar.org> wrote:
>>> @@ -1101,15 +1096,9 @@ drm_output_prepare_overlay_view(struct drm_output *output,
>>>         if (!found)
>>>                 return NULL;
>>>
>>> -       if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
>>> +       if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource)) &&
>>> +           b->no_addfb2 && b->gbm) {
>>>  #ifdef HAVE_GBM_FD_IMPORT
>>> -               /* XXX: TODO:
>>> -                *
>>> -                * Use AddFB2 directly, do not go via GBM.
>>> -                * Add support for multiplanar formats.
>>> -                * Both require refactoring in the DRM-backend to
>>> -                * support a mix of gbm_bos and drmfbs.
>>> -                */
>>>                 struct gbm_import_fd_data gbm_dmabuf = {
>>>                         .fd     = dmabuf->attributes.fd[0],
>>>                         .width  = dmabuf->attributes.width,
>>
>> Here I think there is no benefit to using GBM import: I think it would
>> be more consistent if we always bypassed GBM and just used our own
>> non-GBM drm_fb type. What do you think? If we integrate this patch
>> into the drm_plane_state/atomic tree, the changes for this patch are
>> quite simple: mainly we need to add a BUFFER_DMABUF type to drm_fb,
>> and change the prepare_{overlay_scanout}_view paths to put the new
>> buffer into the plane state directly. I am happy to make this change
>> myself if you think it is OK.
> 
> Unfortunately I have had to pull this patch from my tree. The reason
> is that the handle returned by drmPrimeFDToHandle and closed with
> DRM_IOCTL_GEM_CLOSE is shared but not refcounted.
> 
> GEM drivers have a rule that each buffer _must_ map back to a single
> GEM handle for each DRM connection. So, multiple drmPrimeFDToHandle
> calls from the same client will return the same GEM handle if the
> underlying buffer is the same, even if it is through a different FD.
> The first DRM_IOCTL_GEM_CLOSE call will invalidate that handle
> completely.
> 
> This is a problem for EGL drivers which use dmabuf and GEM: for
> instance, on Mesa, the driver will import the dmabuf and create one
> handle, then we will create another handle from our own dmabuf import
> here, and then later close it from underneath the EGL driver. The EGL
> driver can no longer render anything, since the handle is invalid when
> submitting commands.
> 
> GBM now has a GBM_IMPORT_FD_MODIFIER path, which allows specifying
> multiple planes and format modifiers when importing. So, I have
> rewritten the patch to use this new path instead: as GBM uses the same
> DRM BO cache as EGL does, we no longer have the lifetime issue.
> 

I agree to that.
Thank you for the update.

Cheers,
Esaki