[RFC,2/2] dri: Pass a __DRIcontext to ->set_damage_region()

Submitted by Boris Brezillon on Aug. 29, 2019, 11:54 a.m.

Details

Message ID 20190829115416.15775-2-boris.brezillon@collabora.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Aug. 29, 2019, 11:54 a.m.
So we can call st_validate_state() from dri2_set_damage_region() in
order to update the BACK_LEFT attachement before using it. If we don't
do that, the resource passed to pipe_screen->set_damage_region() might
be wrong.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Hi,

I honestly don't know if this is the right thing to do. Passing a
context for somethings that we decided to bind to a surface (and
the underlying resources attached to it) seems wrong, but at the same
time I don't see any other option to sync the gallium drawable state
with the EGL DRI surface one.

Any ideas/suggestions?

Regards,

Boris
---
 include/GL/internal/dri_interface.h   |  5 +++--
 src/egl/drivers/dri2/egl_dri2.c       |  7 +++++--
 src/gallium/state_trackers/dri/dri2.c | 11 ++++++++++-
 3 files changed, 18 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
index 4c60d349ddd5..e04bed689219 100644
--- a/include/GL/internal/dri_interface.h
+++ b/include/GL/internal/dri_interface.h
@@ -527,12 +527,13 @@  struct __DRI2bufferDamageExtensionRec {
     *
     * Used to implement EGL_KHR_partial_update.
     *
+    * \param ctx      context
     * \param drawable affected drawable
     * \param nrects   number of rectangles provided
     * \param rects    the array of rectangles, lower-left origin
     */
-   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
-                             int *rects);
+   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
+                             unsigned int nrects, int *rects);
 };
 
 /*@}*/
diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index ea8ae5d44c56..797a76dab13c 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -1812,11 +1812,14 @@  dri2_set_damage_region(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf,
 {
    struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
    __DRIdrawable *drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
+   _EGLContext *ctx = _eglGetCurrentContext();
+   __DRIcontext *dri_ctx = ctx ? dri2_egl_context(ctx)->dri_context : NULL;
 
-   if (!dri2_dpy->buffer_damage || !dri2_dpy->buffer_damage->set_damage_region)
+   if (!dri2_dpy->buffer_damage || !dri2_dpy->buffer_damage->set_damage_region ||
+       !dri_ctx)
       return EGL_FALSE;
 
-   dri2_dpy->buffer_damage->set_damage_region(drawable, n_rects, rects);
+   dri2_dpy->buffer_damage->set_damage_region(dri_ctx, drawable, n_rects, rects);
    return EGL_TRUE;
 }
 
diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
index 11ce19787249..bab503046510 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -36,6 +36,7 @@ 
 #include "util/u_format.h"
 #include "util/u_debug.h"
 #include "state_tracker/drm_driver.h"
+#include "state_tracker/st_atom.h"
 #include "state_tracker/st_cb_bufferobjects.h"
 #include "state_tracker/st_cb_fbo.h"
 #include "state_tracker/st_cb_texture.h"
@@ -1869,9 +1870,17 @@  static const __DRI2interopExtension dri2InteropExtension = {
  * \brief the DRI2bufferDamageExtension set_damage_region method
  */
 static void
-dri2_set_damage_region(__DRIdrawable *dPriv, unsigned int nrects, int *rects)
+dri2_set_damage_region(__DRIcontext *_ctx, __DRIdrawable *dPriv,
+                       unsigned int nrects, int *rects)
 {
    struct dri_drawable *drawable = dri_drawable(dPriv);
+   struct st_context *st = (struct st_context *)dri_context(_ctx)->st;
+
+   /* Make sure drawable->textures[ST_ATTACHMENT_BACK_LEFT] is up-to-date
+    * before using it.
+    */
+   st_validate_state(st, ST_PIPELINE_UPDATE_FRAMEBUFFER);
+
    struct pipe_resource *resource = drawable->textures[ST_ATTACHMENT_BACK_LEFT];
    struct pipe_screen *screen = resource->screen;
    struct pipe_box *boxes = NULL;

Comments

On Thu, 29 Aug 2019 13:54:16 +0200 Boris Brezillon
<boris.brezillon@collabora.com> said:

These 2 do improve things, but once you start doing BindFramebuffer()'s as part
of the render cycle ... its back to rendering artifacts. I am not quite sure
exactly what yet. I need to capture some output and traces to get a better idea
of what is going on.

> So we can call st_validate_state() from dri2_set_damage_region() in
> order to update the BACK_LEFT attachement before using it. If we don't
> do that, the resource passed to pipe_screen->set_damage_region() might
> be wrong.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Hi,
> 
> I honestly don't know if this is the right thing to do. Passing a
> context for somethings that we decided to bind to a surface (and
> the underlying resources attached to it) seems wrong, but at the same
> time I don't see any other option to sync the gallium drawable state
> with the EGL DRI surface one.
> 
> Any ideas/suggestions?
> 
> Regards,
> 
> Boris
> ---
>  include/GL/internal/dri_interface.h   |  5 +++--
>  src/egl/drivers/dri2/egl_dri2.c       |  7 +++++--
>  src/gallium/state_trackers/dri/dri2.c | 11 ++++++++++-
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h
> b/include/GL/internal/dri_interface.h index 4c60d349ddd5..e04bed689219 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
>      *
>      * Used to implement EGL_KHR_partial_update.
>      *
> +    * \param ctx      context
>      * \param drawable affected drawable
>      * \param nrects   number of rectangles provided
>      * \param rects    the array of rectangles, lower-left origin
>      */
> -   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> -                             int *rects);
> +   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> +                             unsigned int nrects, int *rects);
>  };
>  
>  /*@}*/
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index ea8ae5d44c56..797a76dab13c 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -1812,11 +1812,14 @@ dri2_set_damage_region(_EGLDriver *drv, _EGLDisplay
> *disp, _EGLSurface *surf, {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>     __DRIdrawable *drawable = dri2_dpy->vtbl->get_dri_drawable(surf);
> +   _EGLContext *ctx = _eglGetCurrentContext();
> +   __DRIcontext *dri_ctx = ctx ? dri2_egl_context(ctx)->dri_context : NULL;
>  
> -   if (!dri2_dpy->buffer_damage || !
> dri2_dpy->buffer_damage->set_damage_region)
> +   if (!dri2_dpy->buffer_damage || !
> dri2_dpy->buffer_damage->set_damage_region ||
> +       !dri_ctx)
>        return EGL_FALSE;
>  
> -   dri2_dpy->buffer_damage->set_damage_region(drawable, n_rects, rects);
> +   dri2_dpy->buffer_damage->set_damage_region(dri_ctx, drawable, n_rects,
> rects); return EGL_TRUE;
>  }
>  
> diff --git a/src/gallium/state_trackers/dri/dri2.c
> b/src/gallium/state_trackers/dri/dri2.c index 11ce19787249..bab503046510
> 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -36,6 +36,7 @@
>  #include "util/u_format.h"
>  #include "util/u_debug.h"
>  #include "state_tracker/drm_driver.h"
> +#include "state_tracker/st_atom.h"
>  #include "state_tracker/st_cb_bufferobjects.h"
>  #include "state_tracker/st_cb_fbo.h"
>  #include "state_tracker/st_cb_texture.h"
> @@ -1869,9 +1870,17 @@ static const __DRI2interopExtension
> dri2InteropExtension = {
>   * \brief the DRI2bufferDamageExtension set_damage_region method
>   */
>  static void
> -dri2_set_damage_region(__DRIdrawable *dPriv, unsigned int nrects, int *rects)
> +dri2_set_damage_region(__DRIcontext *_ctx, __DRIdrawable *dPriv,
> +                       unsigned int nrects, int *rects)
>  {
>     struct dri_drawable *drawable = dri_drawable(dPriv);
> +   struct st_context *st = (struct st_context *)dri_context(_ctx)->st;
> +
> +   /* Make sure drawable->textures[ST_ATTACHMENT_BACK_LEFT] is up-to-date
> +    * before using it.
> +    */
> +   st_validate_state(st, ST_PIPELINE_UPDATE_FRAMEBUFFER);
> +
>     struct pipe_resource *resource =
> drawable->textures[ST_ATTACHMENT_BACK_LEFT]; struct pipe_screen *screen =
> resource->screen; struct pipe_box *boxes = NULL;
> -- 
> 2.21.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 2019-08-29 1:54 p.m., Boris Brezillon wrote:
> So we can call st_validate_state() from dri2_set_damage_region() in
> order to update the BACK_LEFT attachement before using it. If we don't
> do that, the resource passed to pipe_screen->set_damage_region() might
> be wrong.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Hi,
> 
> I honestly don't know if this is the right thing to do. Passing a
> context for somethings that we decided to bind to a surface (and
> the underlying resources attached to it) seems wrong, but at the same
> time I don't see any other option to sync the gallium drawable state
> with the EGL DRI surface one.
> 
> Any ideas/suggestions?
> 
> Regards,
> 
> Boris
> ---
>  include/GL/internal/dri_interface.h   |  5 +++--
>  src/egl/drivers/dri2/egl_dri2.c       |  7 +++++--
>  src/gallium/state_trackers/dri/dri2.c | 11 ++++++++++-
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index 4c60d349ddd5..e04bed689219 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
>      *
>      * Used to implement EGL_KHR_partial_update.
>      *
> +    * \param ctx      context
>      * \param drawable affected drawable
>      * \param nrects   number of rectangles provided
>      * \param rects    the array of rectangles, lower-left origin
>      */
> -   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> -                             int *rects);
> +   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> +                             unsigned int nrects, int *rects);
>  };

This would break the DRI2_BufferDamage extension version 1 ABI. You'd
need to either add a new hook like set_damage_region2 and bump
__DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
everywhere), or add a new extension instead.
On Fri, 30 Aug 2019 18:43:59 +0200
Michel Dänzer <michel@daenzer.net> wrote:

> On 2019-08-29 1:54 p.m., Boris Brezillon wrote:
> > So we can call st_validate_state() from dri2_set_damage_region() in
> > order to update the BACK_LEFT attachement before using it. If we don't
> > do that, the resource passed to pipe_screen->set_damage_region() might
> > be wrong.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > Hi,
> > 
> > I honestly don't know if this is the right thing to do. Passing a
> > context for somethings that we decided to bind to a surface (and
> > the underlying resources attached to it) seems wrong, but at the same
> > time I don't see any other option to sync the gallium drawable state
> > with the EGL DRI surface one.
> > 
> > Any ideas/suggestions?
> > 
> > Regards,
> > 
> > Boris
> > ---
> >  include/GL/internal/dri_interface.h   |  5 +++--
> >  src/egl/drivers/dri2/egl_dri2.c       |  7 +++++--
> >  src/gallium/state_trackers/dri/dri2.c | 11 ++++++++++-
> >  3 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> > index 4c60d349ddd5..e04bed689219 100644
> > --- a/include/GL/internal/dri_interface.h
> > +++ b/include/GL/internal/dri_interface.h
> > @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> >      *
> >      * Used to implement EGL_KHR_partial_update.
> >      *
> > +    * \param ctx      context
> >      * \param drawable affected drawable
> >      * \param nrects   number of rectangles provided
> >      * \param rects    the array of rectangles, lower-left origin
> >      */
> > -   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> > -                             int *rects);
> > +   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> > +                             unsigned int nrects, int *rects);
> >  };  
> 
> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
> need to either add a new hook like set_damage_region2 and bump
> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
> everywhere), or add a new extension instead.
> 
> 

I thought this change was only impacting the internal API, but maybe
I'm missing something.
In any case, this extension has been merged recently (mesa-19.2.0-rc1),
so maybe we can fix it before 19.2 is released to avoid creating
->set_damage_region2().
On 2019-08-30 6:52 p.m., Boris Brezillon wrote:
> On Fri, 30 Aug 2019 18:43:59 +0200
> Michel Dänzer <michel@daenzer.net> wrote:
> 
>>> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
>>> index 4c60d349ddd5..e04bed689219 100644
>>> --- a/include/GL/internal/dri_interface.h
>>> +++ b/include/GL/internal/dri_interface.h
>>> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
>>>      *
>>>      * Used to implement EGL_KHR_partial_update.
>>>      *
>>> +    * \param ctx      context
>>>      * \param drawable affected drawable
>>>      * \param nrects   number of rectangles provided
>>>      * \param rects    the array of rectangles, lower-left origin
>>>      */
>>> -   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
>>> -                             int *rects);
>>> +   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
>>> +                             unsigned int nrects, int *rects);
>>>  };  
>>
>> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
>> need to either add a new hook like set_damage_region2 and bump
>> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
>> everywhere), or add a new extension instead.
> 
> I thought this change was only impacting the internal API, but maybe
> I'm missing something.

include/GL/internal/dri_interface.h defines the DRI driver ABI, which
must be kept backwards compatible.


> In any case, this extension has been merged recently (mesa-19.2.0-rc1),
> so maybe we can fix it before 19.2 is released to avoid creating
> ->set_damage_region2().

Ah, yes. I misinterpreted gitk's output, thinking it had already been
introduced in 19.1. Sorry for the false alarm.
On Fri, 30 Aug 2019 18:56:52 +0200
Michel Dänzer <michel@daenzer.net> wrote:

> On 2019-08-30 6:52 p.m., Boris Brezillon wrote:
> > On Fri, 30 Aug 2019 18:43:59 +0200
> > Michel Dänzer <michel@daenzer.net> wrote:
> >   
> >>> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> >>> index 4c60d349ddd5..e04bed689219 100644
> >>> --- a/include/GL/internal/dri_interface.h
> >>> +++ b/include/GL/internal/dri_interface.h
> >>> @@ -527,12 +527,13 @@ struct __DRI2bufferDamageExtensionRec {
> >>>      *
> >>>      * Used to implement EGL_KHR_partial_update.
> >>>      *
> >>> +    * \param ctx      context
> >>>      * \param drawable affected drawable
> >>>      * \param nrects   number of rectangles provided
> >>>      * \param rects    the array of rectangles, lower-left origin
> >>>      */
> >>> -   void (*set_damage_region)(__DRIdrawable *drawable, unsigned int nrects,
> >>> -                             int *rects);
> >>> +   void (*set_damage_region)(__DRIcontext *_ctx, __DRIdrawable *drawable,
> >>> +                             unsigned int nrects, int *rects);
> >>>  };    
> >>
> >> This would break the DRI2_BufferDamage extension version 1 ABI. You'd
> >> need to either add a new hook like set_damage_region2 and bump
> >> __DRI2_BUFFER_DAMAGE_VERSION (and make sure that's handled correctly
> >> everywhere), or add a new extension instead.  
> > 
> > I thought this change was only impacting the internal API, but maybe
> > I'm missing something.  
> 
> include/GL/internal/dri_interface.h defines the DRI driver ABI, which
> must be kept backwards compatible.

Okay.

> 
> 
> > In any case, this extension has been merged recently (mesa-19.2.0-rc1),
> > so maybe we can fix it before 19.2 is released to avoid creating  
> > ->set_damage_region2().  
> 
> Ah, yes. I misinterpreted gitk's output, thinking it had already been
> introduced in 19.1. Sorry for the false alarm.

Great! So, next question is, do you think it's acceptable to pass a
DRIcontext here, and if not, do you have any idea how to solve this
problem?
On 2019-08-30 7:00 p.m., Boris Brezillon wrote:
> 
> So, next question is, do you think it's acceptable to pass a
> DRIcontext here, and if not, do you have any idea how to solve this
> problem?

Hmm, not sure. Maybe it would be better to explicitly pass in the
__DRIimage* to which the damage region applies?