Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On 07/01/2015 10:24, Joonas Lahtinenwrote : This is still awaiting for comments. I'd rather hear what are the desirable modifications than try guessing. Well, ideally you would implement DRI3/Present support instead of complementing DRI2 support with Present. Why improve the old solution, instead of switching to the new one ? I know DRI3 is having a few issues to get support because of a few bugs in the stack, but if what you want is just small improvement to reduce overhead, then I woud think the answer is more implement that feature with DRI3. Axel Davy ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
This is still awaiting for comments. I'd rather hear what are the desirable modifications than try guessing. On ma, 2014-11-10 at 15:18 +0200, Joonas Lahtinen wrote: > Hi, > > On pe, 2014-11-07 at 17:40 -0800, Eric Anholt wrote: > > Ian Romanick writes: > > > > > On 11/06/2014 06:16 PM, Michel Dänzer wrote: > > >> On 06.11.2014 19:18, Joonas Lahtinen wrote: > > >>> On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: > > On 05.11.2014 20:14, Joonas Lahtinen wrote: > > > > > > Modified not refer to DRI3, just uses the present extension to get rid > > > of the excess buffer invalidations. > > > > AFAICT there's no fallback from your changes to the current behaviour > > if > > the X server doesn't support the Present extension. There probably > > needs > > to be such a fallback. > > >>> > > >>> It gets rid of such nasty hack (the intel_viewport one), that I thought > > >>> there is no point making fallback. Because without this, the egl dri2 > > >>> backend is fundamentally broken anyway. > > >> > > >> Well, AFAICT your code uses Present extension functionality > > >> unconditionally, without checking that the X server supports Present. I > > >> can't see how that could possibly work on an X server which doesn't > > >> support Present, but I think it would be better to keep it working at > > >> least as badly as it does now in that case. :) > > > > > > I was going to say pretty much the same thing. Aren't there (non-Intel) > > > drivers that don't do Present? If I'm not mistaken, some parts of DRI3 > > > (not sure about Present) are even disabled in the Intel driver when SNA > > > is in use... or at least that was the case at one point. > > > > They actually get a fallback implementation if there's no driver > > support, which would be sufficient for this code. > > > > However, Present is too new for Mesa to be unconditionally relying on in > > my opinion. > > Based on above discussion, I would bring back the dynamic detection like > in the original patch. But for present extension instead of DRI3. > Technically it would be very much the same, different naming > conventions. And also, re-use the USE_INVALIDATE extension instead of > adding DRI3 extension. > > Would that be an acceptable solution? > > Regards, Joonas > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
Hi, On pe, 2014-11-07 at 17:40 -0800, Eric Anholt wrote: > Ian Romanick writes: > > > On 11/06/2014 06:16 PM, Michel Dänzer wrote: > >> On 06.11.2014 19:18, Joonas Lahtinen wrote: > >>> On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: > On 05.11.2014 20:14, Joonas Lahtinen wrote: > > > > Modified not refer to DRI3, just uses the present extension to get rid > > of the excess buffer invalidations. > > AFAICT there's no fallback from your changes to the current behaviour if > the X server doesn't support the Present extension. There probably needs > to be such a fallback. > >>> > >>> It gets rid of such nasty hack (the intel_viewport one), that I thought > >>> there is no point making fallback. Because without this, the egl dri2 > >>> backend is fundamentally broken anyway. > >> > >> Well, AFAICT your code uses Present extension functionality > >> unconditionally, without checking that the X server supports Present. I > >> can't see how that could possibly work on an X server which doesn't > >> support Present, but I think it would be better to keep it working at > >> least as badly as it does now in that case. :) > > > > I was going to say pretty much the same thing. Aren't there (non-Intel) > > drivers that don't do Present? If I'm not mistaken, some parts of DRI3 > > (not sure about Present) are even disabled in the Intel driver when SNA > > is in use... or at least that was the case at one point. > > They actually get a fallback implementation if there's no driver > support, which would be sufficient for this code. > > However, Present is too new for Mesa to be unconditionally relying on in > my opinion. Based on above discussion, I would bring back the dynamic detection like in the original patch. But for present extension instead of DRI3. Technically it would be very much the same, different naming conventions. And also, re-use the USE_INVALIDATE extension instead of adding DRI3 extension. Would that be an acceptable solution? Regards, Joonas ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
Ian Romanick writes: > On 11/06/2014 06:16 PM, Michel Dänzer wrote: >> On 06.11.2014 19:18, Joonas Lahtinen wrote: >>> On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: On 05.11.2014 20:14, Joonas Lahtinen wrote: > > Modified not refer to DRI3, just uses the present extension to get rid > of the excess buffer invalidations. AFAICT there's no fallback from your changes to the current behaviour if the X server doesn't support the Present extension. There probably needs to be such a fallback. >>> >>> It gets rid of such nasty hack (the intel_viewport one), that I thought >>> there is no point making fallback. Because without this, the egl dri2 >>> backend is fundamentally broken anyway. >> >> Well, AFAICT your code uses Present extension functionality >> unconditionally, without checking that the X server supports Present. I >> can't see how that could possibly work on an X server which doesn't >> support Present, but I think it would be better to keep it working at >> least as badly as it does now in that case. :) > > I was going to say pretty much the same thing. Aren't there (non-Intel) > drivers that don't do Present? If I'm not mistaken, some parts of DRI3 > (not sure about Present) are even disabled in the Intel driver when SNA > is in use... or at least that was the case at one point. They actually get a fallback implementation if there's no driver support, which would be sufficient for this code. However, Present is too new for Mesa to be unconditionally relying on in my opinion. pgpD4o39VLXLa.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On 11/06/2014 06:16 PM, Michel Dänzer wrote: > On 06.11.2014 19:18, Joonas Lahtinen wrote: >> On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: >>> On 05.11.2014 20:14, Joonas Lahtinen wrote: Modified not refer to DRI3, just uses the present extension to get rid of the excess buffer invalidations. >>> >>> AFAICT there's no fallback from your changes to the current behaviour if >>> the X server doesn't support the Present extension. There probably needs >>> to be such a fallback. >> >> It gets rid of such nasty hack (the intel_viewport one), that I thought >> there is no point making fallback. Because without this, the egl dri2 >> backend is fundamentally broken anyway. > > Well, AFAICT your code uses Present extension functionality > unconditionally, without checking that the X server supports Present. I > can't see how that could possibly work on an X server which doesn't > support Present, but I think it would be better to keep it working at > least as badly as it does now in that case. :) I was going to say pretty much the same thing. Aren't there (non-Intel) drivers that don't do Present? If I'm not mistaken, some parts of DRI3 (not sure about Present) are even disabled in the Intel driver when SNA is in use... or at least that was the case at one point. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On 06.11.2014 19:18, Joonas Lahtinen wrote: On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: On 05.11.2014 20:14, Joonas Lahtinen wrote: Modified not refer to DRI3, just uses the present extension to get rid of the excess buffer invalidations. AFAICT there's no fallback from your changes to the current behaviour if the X server doesn't support the Present extension. There probably needs to be such a fallback. It gets rid of such nasty hack (the intel_viewport one), that I thought there is no point making fallback. Because without this, the egl dri2 backend is fundamentally broken anyway. Well, AFAICT your code uses Present extension functionality unconditionally, without checking that the X server supports Present. I can't see how that could possibly work on an X server which doesn't support Present, but I think it would be better to keep it working at least as badly as it does now in that case. :) -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On ke, 2014-11-05 at 15:19 +, Emil Velikov wrote:
> Hi Joonas,
>
> Does getting rid of the viewport hack give you any noticeable
> performance improvement?
Yes, it significantly reduces the CPU load when multiple glViewport
calls are made per frame (4x4 grid or so).
> Is there any interest in converting the
> egl_dri2 backend to dri3, rather than just copying over the present bits ?
>
This could be one thing to do. But in the meanwhile, I would commit this
present extension patch so that the affected use cases get the
improvements.
Regards, Joonas
> On 05/11/14 11:14, Joonas Lahtinen wrote:
> > Hi,
> >
> > Modified not refer to DRI3, just uses the present extension to get rid
> > of the excess buffer invalidations.
> >
> > Regards, Joonas
> >
> > From 257e2a8c750f9dcf868cce9da8632df3cae0fcec Mon Sep 17 00:00:00 2001
> > From: Joonas Lahtinen
> > Date: Wed, 5 Nov 2014 12:25:32 +0200
> > Subject: [PATCH] egl: dri2: Use present extension.
> >
> > Present extension is used to avoid excess buffer invalidations, because
> > the XCB interface doesn't supply that information.
> >
> > Signed-off-by: Daniel van der Wath
> > Signed-off-by: Joonas Lahtinen
> > ---
> > configure.ac|5 +-
> > src/egl/drivers/dri2/egl_dri2.c |2 +-
> > src/egl/drivers/dri2/egl_dri2.h | 24 ++-
> > src/egl/drivers/dri2/platform_x11.c | 247
> > ---
> > src/mesa/drivers/dri/i965/brw_context.c |9 +-
> > 5 files changed, 262 insertions(+), 25 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index fc7d372..75d90c0 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -952,7 +952,8 @@ xyesno)
> > fi
> >
> > if test x"$enable_dri" = xyes; then
> > - dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED"
> > + PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >=
> > $PRESENTPROTO_REQUIRED])
> > + dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED
> > xcb-present"
> Afaics you are not changing anything on the dri modules (or glx/dri2) to
> require the above changes. Perhaps you need to push the presentproto
> check in the x11 case below ?
>
> > fi
> >
> > if test x"$enable_dri3" = xyes; then
> > @@ -1564,7 +1565,7 @@ for plat in $egl_platforms; do
> > ;;
> >
> > x11)
> > - PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >=
> > $XCBDRI2_REQUIRED xcb-xfixes])
> > + PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >=
> > $XCBDRI2_REQUIRED xcb-xfixes xcb-present])
> > ;;
> >
> > drm)
> [snip]
> > diff --git a/src/egl/drivers/dri2/platform_x11.c
> > b/src/egl/drivers/dri2/platform_x11.c
> > index f8c4b70..a1445b2 100644
> > --- a/src/egl/drivers/dri2/platform_x11.c
> > +++ b/src/egl/drivers/dri2/platform_x11.c
> > @@ -188,6 +188,205 @@ get_xcb_screen(xcb_screen_iterator_t iter, int screen)
> > return NULL;
> > }
> >
> > +/*
> > + * Called by the XCB_PRESENT_COMPLETE_NOTIFY case.
> > + */
> > +static void
> > +dri2_update_num_back(struct dri2_egl_surface *priv)
> > +{
> > + priv->num_back = 1;
> > + if (priv->flipping)
> > + priv->num_back++;
> > + if (priv->base.SwapInterval == 0)
> > + priv->num_back++;
> > +}
> > +
> This seems to be out of sync with dri3_glx. Don't you need something
> similar to commit f7a36ef5fe23056299a77414f9ad8b5e5a1d ?
>
> [snip]
> > +/**
> > + *
> > + * Process any present events that have been received from the X server
> > + *
> > + * From glx, we additionally invalidate the drawable here if there has a
> > been a special event.
> > + */
> > +static void
> > +dri2_flush_present_events(struct dri2_egl_display *dri2_dpy, struct
> > dri2_egl_surface *priv)
> > +{
> > + xcb_connection_t *c = dri2_dpy->conn;
> > +
> > + /* Check to see if any configuration changes have occurred
> > +* since we were last invoked
> > +*/
> > + if (priv->special_event) {
> > + xcb_generic_event_t*ev;
> > +
> > + while ((ev = xcb_poll_for_special_event(c, priv->special_event)) !=
> > NULL) {
> > + xcb_present_generic_event_t *ge = (void *) ev;
> > + dri2_handle_present_event(priv, ge);
> > + _eglLog(_EGL_INFO, "DRI: Invalidating buffer 0x%x\n",
> > priv->dri_drawable);
> > + (*dri2_dpy->flush->invalidate)(priv->dri_drawable);
> Hmm why does one need to invalidate at this stage - I take that it's
> related to lack of fence objects ?
>
> [snip]
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> > b/src/mesa/drivers/dri/i965/brw_context.c
> > index e1a994a..dbadd10 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -148,6 +148,9 @@ intel_viewport(struct gl_context *ctx)
> > __DRIcontext *driContext = brw->driContext;
> >
> > if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> >
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On to, 2014-11-06 at 18:12 +0900, Michel Dänzer wrote: > On 05.11.2014 20:14, Joonas Lahtinen wrote: > > > > Modified not refer to DRI3, just uses the present extension to get rid > > of the excess buffer invalidations. > > AFAICT there's no fallback from your changes to the current behaviour if > the X server doesn't support the Present extension. There probably needs > to be such a fallback. > > It gets rid of such nasty hack (the intel_viewport one), that I thought there is no point making fallback. Because without this, the egl dri2 backend is fundamentally broken anyway. Regards, Joonas ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
On 05.11.2014 20:14, Joonas Lahtinen wrote: Modified not refer to DRI3, just uses the present extension to get rid of the excess buffer invalidations. AFAICT there's no fallback from your changes to the current behaviour if the X server doesn't support the Present extension. There probably needs to be such a fallback. -- Earthling Michel Dänzer| http://www.amd.com Libre software enthusiast |Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: dri2: Use present extension. (Was: Re: [RFC] egl: Add DRI3 support to the EGL backend.)
Hi Joonas,
Does getting rid of the viewport hack give you any noticeable
performance improvement ? Is there any interest in converting the
egl_dri2 backend to dri3, rather than just copying over the present bits ?
On 05/11/14 11:14, Joonas Lahtinen wrote:
> Hi,
>
> Modified not refer to DRI3, just uses the present extension to get rid
> of the excess buffer invalidations.
>
> Regards, Joonas
>
> From 257e2a8c750f9dcf868cce9da8632df3cae0fcec Mon Sep 17 00:00:00 2001
> From: Joonas Lahtinen
> Date: Wed, 5 Nov 2014 12:25:32 +0200
> Subject: [PATCH] egl: dri2: Use present extension.
>
> Present extension is used to avoid excess buffer invalidations, because
> the XCB interface doesn't supply that information.
>
> Signed-off-by: Daniel van der Wath
> Signed-off-by: Joonas Lahtinen
> ---
> configure.ac|5 +-
> src/egl/drivers/dri2/egl_dri2.c |2 +-
> src/egl/drivers/dri2/egl_dri2.h | 24 ++-
> src/egl/drivers/dri2/platform_x11.c | 247
> ---
> src/mesa/drivers/dri/i965/brw_context.c |9 +-
> 5 files changed, 262 insertions(+), 25 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index fc7d372..75d90c0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -952,7 +952,8 @@ xyesno)
> fi
>
> if test x"$enable_dri" = xyes; then
> - dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED"
> + PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >=
> $PRESENTPROTO_REQUIRED])
> + dri_modules="$dri_modules xcb-dri2 >= $XCBDRI2_REQUIRED
> xcb-present"
Afaics you are not changing anything on the dri modules (or glx/dri2) to
require the above changes. Perhaps you need to push the presentproto
check in the x11 case below ?
> fi
>
> if test x"$enable_dri3" = xyes; then
> @@ -1564,7 +1565,7 @@ for plat in $egl_platforms; do
> ;;
>
> x11)
> - PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >=
> $XCBDRI2_REQUIRED xcb-xfixes])
> + PKG_CHECK_MODULES([XCB_DRI2], [x11-xcb xcb xcb-dri2 >=
> $XCBDRI2_REQUIRED xcb-xfixes xcb-present])
> ;;
>
> drm)
[snip]
> diff --git a/src/egl/drivers/dri2/platform_x11.c
> b/src/egl/drivers/dri2/platform_x11.c
> index f8c4b70..a1445b2 100644
> --- a/src/egl/drivers/dri2/platform_x11.c
> +++ b/src/egl/drivers/dri2/platform_x11.c
> @@ -188,6 +188,205 @@ get_xcb_screen(xcb_screen_iterator_t iter, int screen)
> return NULL;
> }
>
> +/*
> + * Called by the XCB_PRESENT_COMPLETE_NOTIFY case.
> + */
> +static void
> +dri2_update_num_back(struct dri2_egl_surface *priv)
> +{
> + priv->num_back = 1;
> + if (priv->flipping)
> + priv->num_back++;
> + if (priv->base.SwapInterval == 0)
> + priv->num_back++;
> +}
> +
This seems to be out of sync with dri3_glx. Don't you need something
similar to commit f7a36ef5fe23056299a77414f9ad8b5e5a1d ?
[snip]
> +/**
> + *
> + * Process any present events that have been received from the X server
> + *
> + * From glx, we additionally invalidate the drawable here if there has a
> been a special event.
> + */
> +static void
> +dri2_flush_present_events(struct dri2_egl_display *dri2_dpy, struct
> dri2_egl_surface *priv)
> +{
> + xcb_connection_t *c = dri2_dpy->conn;
> +
> + /* Check to see if any configuration changes have occurred
> +* since we were last invoked
> +*/
> + if (priv->special_event) {
> + xcb_generic_event_t*ev;
> +
> + while ((ev = xcb_poll_for_special_event(c, priv->special_event)) !=
> NULL) {
> + xcb_present_generic_event_t *ge = (void *) ev;
> + dri2_handle_present_event(priv, ge);
> + _eglLog(_EGL_INFO, "DRI: Invalidating buffer 0x%x\n",
> priv->dri_drawable);
> + (*dri2_dpy->flush->invalidate)(priv->dri_drawable);
Hmm why does one need to invalidate at this stage - I take that it's
related to lack of fence objects ?
[snip]
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> index e1a994a..dbadd10 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -148,6 +148,9 @@ intel_viewport(struct gl_context *ctx)
> __DRIcontext *driContext = brw->driContext;
>
> if (_mesa_is_winsys_fbo(ctx->DrawBuffer)) {
> + if (unlikely(INTEL_DEBUG & DEBUG_DRI))
> + fprintf(stderr, "invalidating drawables\n");
> +
>dri2InvalidateDrawable(driContext->driDrawablePriv);
>dri2InvalidateDrawable(driContext->driReadablePriv);
> }
> @@ -252,11 +255,9 @@ brw_init_driver_functions(struct brw_context *brw,
> _mesa_init_driver_functions(functions);
>
> /* GLX uses DRI2 invalidate events to handle window resizing.
> -* Unfortunately, EGL does not - libEGL is written in XCB (not Xlib),
> -* which doesn't provide a mechanism for snooping the event queues.
> +
