[Mesa-dev] [PATCH 02/14] vl/dri3: implement dri3 screen create and destroy
Emil Velikov
emil.l.velikov at gmail.com
Thu May 12 15:08:21 UTC 2016
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.
>>
>>>>> + 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.
Thanks
Emil
More information about the mesa-dev
mailing list