[Mesa-dev] [PATCH 02/14] vl/dri3: implement dri3 screen create and destroy
Leo Liu
leo.liu at amd.com
Thu May 12 15:25:22 UTC 2016
On 05/12/2016 11:08 AM, Emil Velikov wrote:
> On 12 May 2016 at 15:01, Leo Liu <leo.liu at amd.com> wrote:
>>
>> On 05/12/2016 09:47 AM, Emil Velikov wrote:
>>> 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 ?
>>
>> Yes I think so. That's for all vl, includes building vl_winsys_dri.c.
>>
>>
> Yes we need it, or yes we don't need it ? Afaict the vl_winsys_dri.c
> case is handled in the else statement.
We still need vl_winsys_dri.c even with "enable_dri3", because there's
fallback case.
>>>>>> + 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 ?
>> Sure. patch for that will follow up shortly.
> Great.
>
> Note to self: Seems like I've 'butchered' it - i.e. we're doing
> close(-1) elsewhere. Although strictly not forbidden in the manpage,
> better to avoid doing so. I'll send patches for the rest in a bit.
>
>>> Also please squash this patch and 01/14.
>> patch 1 is to set infrastructure, and patch 2 and beyond is for
>> implementation.
>>
> Actually the configure hunk (dependencies) are part of the infra :-)
>
> So if you want to keep these separate please, either introduce the new
> dependencies (in configure.ac) as they are needed (like you've nicely
> done in the code) or add them all in one go with patch 01, please ?
> The latter might be less annoying, personally I'd go with that one.
Sure. I am with you.
Thanks,
Leo
> Thanks
> Emil
More information about the mesa-dev
mailing list