[Mesa-dev] [PATCH 02/14] vl/dri3: implement dri3 screen create and destroy

[Emil Velikov]

Hi Leo,

On 11 May 2016 at 22:14, Leo Liu <leo.liu at amd.com> wrote:
> On 05/11/2016 04:20 PM, Axel Davy wrote:
>>
>> On 11/05/2016 17:06, Leo Liu wrote:
>>>
>>> Screen created with device fd returned from X server,
>>> also will bail out to DRI2 with certain conditions.
>>>
>>> Signed-off-by: Leo Liu <leo.liu at amd.com>
>>> ---
>>>   configure.ac                              |  7 ++-
>>>   src/gallium/auxiliary/vl/vl_winsys_dri3.c | 88
>>> ++++++++++++++++++++++++++++++-
>>>   2 files changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 023110e..8c3960a 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -1779,7 +1779,12 @@ if test "x$enable_xvmc" = xyes -o \
>>>           "x$enable_vdpau" = xyes -o \
>>>           "x$enable_omx" = xyes -o \
>>>           "x$enable_va" = xyes; then
>>> -    PKG_CHECK_MODULES([VL], [x11-xcb xcb xcb-dri2 >= $XCBDRI2_REQUIRED])
>>> +    if test x"$enable_dri3" = xyes; then
>>> +        PKG_CHECK_MODULES([VL], [xcb-dri3 xcb-present xcb-sync xshmfence
>>> >= $XSHMFENCE_REQUIRED
>>> +                                 x11-xcb xcb xcb-dri2 >=
>>> $XCBDRI2_REQUIRED])
We don't need xcb-dri2 in the above do we ?

>>> +    else
>>> +        PKG_CHECK_MODULES([VL], [x11-xcb xcb xcb-dri2 >=
>>> $XCBDRI2_REQUIRED])
>>> +    fi
>>>       need_gallium_vl_winsys=yes
>>>   fi
>>>   AM_CONDITIONAL(NEED_GALLIUM_VL_WINSYS, test "x$need_gallium_vl_winsys"
>>> = xyes)
>>> diff --git a/src/gallium/auxiliary/vl/vl_winsys_dri3.c
>>> b/src/gallium/auxiliary/vl/vl_winsys_dri3.c
>>> index 2c3d3ae..c018379 100644
>>> --- a/src/gallium/auxiliary/vl/vl_winsys_dri3.c
>>> +++ b/src/gallium/auxiliary/vl/vl_winsys_dri3.c
>>> @@ -25,7 +25,16 @@
>>>    *
>>>
>>> **************************************************************************/
>>>   +#include <fcntl.h>
>>> +
>>> +#include <X11/Xlib-xcb.h>
>>> +#include <xcb/dri3.h>
>>> +#include <xcb/present.h>
>>> +
>>> +#include "loader.h"
>>> +
>>>   #include "pipe/p_screen.h"
>>> +#include "pipe-loader/pipe_loader.h"
>>>     #include "util/u_memory.h"
>>>   #include "vl/vl_winsys.h"
>>> @@ -33,6 +42,8 @@
>>>   struct vl_dri3_screen
>>>   {
>>>      struct vl_screen base;
>>> +   xcb_connection_t *conn;
>>> +   xcb_drawable_t drawable;
>>>   };
>>>     static void
>>> @@ -82,7 +93,14 @@ vl_dri3_screen_get_private(struct vl_screen *vscreen)
>>>   static void
>>>   vl_dri3_screen_destroy(struct vl_screen *vscreen)
>>>   {
>>> -   /* TODO */
>>> +   struct vl_dri3_screen *scrn = (struct vl_dri3_screen *)vscreen;
>>> +
>>> +   assert(vscreen);
>>> +
>>> +   scrn->base.pscreen->destroy(scrn->base.pscreen);
>>> +   pipe_loader_release(&scrn->base.dev, 1);
>>> +   FREE(scrn);
>>> +
>>>      return;
>>>   }
>>>   @@ -90,6 +108,13 @@ struct vl_screen *
>>>   vl_dri3_screen_create(Display *display, int screen)
>>>   {
>>>      struct vl_dri3_screen *scrn;
>>> +   const xcb_query_extension_reply_t *extension;
>>> +   xcb_dri3_open_cookie_t open_cookie;
>>> +   xcb_dri3_open_reply_t *open_reply;
>>> +   xcb_get_geometry_cookie_t geom_cookie;
>>> +   xcb_get_geometry_reply_t *geom_reply;
>>> +   int is_different_gpu;
>>> +   int fd;
>>>        assert(display);
>>>   @@ -97,6 +122,58 @@ vl_dri3_screen_create(Display *display, int screen)
>>>      if (!scrn)
>>>         return NULL;
>>>   +   scrn->conn = XGetXCBConnection(display);
>>> +   if (!scrn->conn)
>>> +      goto free_screen;
>>> +
>>> +   xcb_prefetch_extension_data(scrn->conn , &xcb_dri3_id);
>>> +   xcb_prefetch_extension_data(scrn->conn, &xcb_present_id);
>>> +   extension = xcb_get_extension_data(scrn->conn, &xcb_dri3_id);
>>> +   if (!(extension && extension->present))
>>> +      goto free_screen;
>>> +   extension = xcb_get_extension_data(scrn->conn, &xcb_present_id);
>>> +   if (!(extension && extension->present))
>>> +      goto free_screen;
>>> +
>>> +   open_cookie = xcb_dri3_open(scrn->conn, RootWindow(display, screen),
>>> None);
>>> +   open_reply = xcb_dri3_open_reply(scrn->conn, open_cookie, NULL);
>>> +   if (!open_reply)
>>> +      goto free_screen;
>>> +   if (open_reply->nfd != 1) {
>>> +      free(open_reply);
>>> +      goto free_screen;
>>> +   }
>>> +
>>> +   fd = xcb_dri3_open_reply_fds(scrn->conn, open_reply)[0];
>>> +   if (fd < 0) {
>>> +      free(open_reply);
>>> +      goto free_screen;
>>> +   }
>>> +   fcntl(fd, F_SETFD, FD_CLOEXEC);
>>> +   free(open_reply);
>>> +
>>> +   fd = loader_get_user_preferred_fd(fd, &is_different_gpu);
>>> +   /* TODO support different GPU */
>>> +   if (is_different_gpu)
>>> +      goto free_screen;
>>> +
>>> +   geom_cookie = xcb_get_geometry(scrn->conn, RootWindow(display,
>>> screen));
>>> +   geom_reply = xcb_get_geometry_reply(scrn->conn, geom_cookie, NULL);
>>> +   if (!geom_reply)
>>> +      goto free_screen;
>>> +   /* TODO support depth other than 24 */
>>> +   if (geom_reply->depth != 24) {
>>> +      free(geom_reply);
>>> +      goto free_screen;
>>> +   }
>>> +   free(geom_reply);
>>> +
>>> +   if (pipe_loader_drm_probe_fd(&scrn->base.dev, fd))
>>> +      scrn->base.pscreen = pipe_loader_create_screen(scrn->base.dev);
>>> +
>>> +   if (!scrn->base.pscreen)
>>> +      goto release_pipe;
>>> +
>>>      scrn->base.destroy = vl_dri3_screen_destroy;
>>>      scrn->base.texture_from_drawable =
>>> vl_dri3_screen_texture_from_drawable;
>>>      scrn->base.get_dirty_area = vl_dri3_screen_get_dirty_area;
>>> @@ -106,4 +183,13 @@ vl_dri3_screen_create(Display *display, int screen)
>>>      scrn->base.pscreen->flush_frontbuffer = vl_dri3_flush_frontbuffer;
>>>        return &scrn->base;
>>> +
>>> +release_pipe:
>>> +   if (scrn->base.dev)
>>> +      pipe_loader_release(&scrn->base.dev, 1);
>>> +   fd = -1;
>>> +   close(fd);
>>
>>
>> I assume mistake there ... or is close(-1) supposed to do something
>> specific ?
>
>
> Good catch. it's wrong.
>
> pipe_loader_release do the close for fd.
> Here should be:
> if (scrn->base.dev)
>    pipe_loader_release(&scrn->base.dev, 1);
> else
>    close(fd);
>
> will fix in v2.
>
Seems like I made this silly mistake in vl_winsys_dri.c where this was
copied from. Leo might sending a patch for that one as well please ?

Also please squash this patch and 01/14.

Thanks
Emil